Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5891 closed defect (fixed)

rep_hist_desc_stats_write() segfaults on empty stats

Reported by: ln5 Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The fix for #5508 wasn't complete -- returning NULL from
rep_hist_format_desc_stats() makes rep_hist_desc_stats_write()
segfault.

rep_hist_format_desc_stats() already returned NULL when
!start_of_served_descs_stats_interval but that probably doesn't happen
very often.

NOTE: This code is run only by bridge authoritatives.

Child Tickets

Change History (13)

comment:1 Changed 8 years ago by ln5

Status: newneeds_review

See branch bug5891 in my public repo for a fix.

comment:2 Changed 8 years ago by karsten

Looks like the problem is that rep_hist_format_desc_stats() returns NULL if size == 0. I wonder if it should instead return a string saying it didn't serve a single descriptor, e.g., "served-descs-stats-end 2012-05-15 09:18:27 (86400 s) total=0 unique=0 max=0 q3=0 md=0 q1=0 min=0\n". That's different from not having any statistics for the time period. Btw, the docs also don't say that the function returns NULL if size == 0. So, the better fix would be to fix rep_hist_format_desc_stats(). If I prepare a patch, can you test it?

comment:3 Changed 8 years ago by ln5

That's a good fix but I think we need this change too since
rep_hist_format_desc_stats() still can return NULL.

I'm happy to test a patch.

comment:4 in reply to:  3 ; Changed 8 years ago by karsten

Replying to ln5:

That's a good fix but I think we need this change too since
rep_hist_format_desc_stats() still can return NULL.

Only if we're not collecting stats, and we're checking that case in rep_hist_desc_stats_write() before calling rep_hist_format_desc_stats().

I'm happy to test a patch.

Please see branch task-5891 in my public repo. I stole your changes file. Please tweak as needed.

comment:5 in reply to:  4 ; Changed 8 years ago by ln5

Replying to karsten:

Replying to ln5:

That's a good fix but I think we need this change too since
rep_hist_format_desc_stats() still can return NULL.

Only if we're not collecting stats, and we're checking that case in rep_hist_desc_stats_write() before calling rep_hist_format_desc_stats().

Then you don't mind me adding this assert in rep_hist_desc_stats_write()?

str = rep_hist_format_desc_stats(now);
tor_assert(str != NULL);

Please see branch task-5891 in my public repo. I stole your changes file. Please tweak as needed.

Tested. Works fine. Thanks.

comment:6 in reply to:  5 Changed 8 years ago by karsten

Replying to ln5:

Then you don't mind me adding this assert in rep_hist_desc_stats_write()?

str = rep_hist_format_desc_stats(now);
tor_assert(str != NULL);

Should be fine, IMHO.

Tested. Works fine. Thanks.

Cool! Thanks for testing. :)

comment:7 Changed 8 years ago by ln5

Assertion added, see task-5891.

Regarding the changes file, was it named 'task-' rather than 'bug' on purpose?

comment:8 in reply to:  7 Changed 8 years ago by karsten

Replying to ln5:

Regarding the changes file, was it named 'task-' rather than 'bug' on purpose?

Not at all. I copy-pasted it from gitweb and picked a name. Feel free to change it back.

comment:9 Changed 8 years ago by ln5

Branch task-5891 is ready for review.

comment:10 in reply to:  9 Changed 8 years ago by ln5

Replying to ln5:

Branch task-5891 is ready for review.

Branch task-5891 in repo linus/tor.git, that is.

comment:11 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merging; thanks!

comment:12 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:13 Changed 7 years ago by nickm

Component: Tor Directory AuthorityTor
Note: See TracTickets for help on using tickets.