Opened 3 years ago

Closed 3 years ago

Last modified 15 months ago

#19130 closed defect (fixed)

Seg fault in round_int64_to_next_multiple_of()

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Critical Keywords: TorCoreTeam201605
Cc: amj703, dgoulet, asn Actual Points: .3
Parent ID: #23414 Points: .3
Reviewer: teor Sponsor: SponsorS-can

Description

On moria1, git 249f3a16

#0  0x00007f8412479625 in raise () from /lib64/libc.so.6
#1  0x00007f841247ae05 in abort () from /lib64/libc.so.6
#2  0x00007f8413caee1d in __subvdi3 ()
#3  0x00007f8413c5ddc0 in round_int64_to_next_multiple_of (number=72742,
    divisor=1024) at src/common/util.c:523
#4  0x00007f8413b7d004 in rep_hist_format_hs_stats (now=1463682482)
    at src/or/rephist.c:3074
#5  rep_hist_hs_stats_write (now=1463682482) at src/or/rephist.c:3122
#6  0x00007f8413b4b3f8 in write_stats_file_callback (now=1463682482,
    options=0x7f84146c6880) at src/or/main.c:1761
#7  0x00007f8413b60ef0 in periodic_event_dispatch (fd=<value optimized out>,
    what=<value optimized out>, data=0x7f8413f4b260) at src/or/periodic.c:52
#8  0x00007f841323cb44 in event_base_loop () from /usr/lib64/libevent-1.4.so.2
#9  0x00007f8413b48e2a in run_main_loop_once () at src/or/main.c:2548
#10 run_main_loop_until_done () at src/or/main.c:2592
#11 do_main_loop () at src/or/main.c:2520
#12 0x00007f8413b49ec5 in tor_main (argc=<value optimized out>,
    argv=<value optimized out>) at src/or/main.c:3647
#13 0x00007f8413b45ec9 in main (argc=<value optimized out>,
    argv=<value optimized out>) at src/or/tor_main.c:30
(gdb) up
#1  0x00007f841247ae05 in abort () from /lib64/libc.so.6
(gdb) up
#2  0x00007f8413caee1d in __subvdi3 ()
(gdb) up
#3  0x00007f8413c5ddc0 in round_int64_to_next_multiple_of (number=72742,
    divisor=1024) at src/common/util.c:523
523       if (INT64_MAX - divisor + 1 < number)

Looks like it's in the "add noise when reporting the number of onion addresses it's seen" area.

I do have a couple of local patches applied to moria1, but "surely" they aren't interacting with this bug.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by asn

Yawning suggests this is a signed int overflow that leads to an abort because of ftrapv (#17983).

The overflow happens at:

  if (INT64_MAX - divisor + 1 < number)
    return INT64_MAX;

whose left side probably gets applied as INT64_MAX + 1 - divisor.

A potential fix here would be to reorder that if statement to:

  if (INT64_MAX - number < divisor - 1)
    return INT64_MAX;

maybe with an additional check that divisor >= 1.

comment:2 Changed 3 years ago by asn

(It might also be worth considering what happened in the past before ftrapv. Did that check actually ever overflow and return INT64_MAX? Maybe in some platforms? This could in theory influence HS stats.)

comment:3 in reply to:  1 Changed 3 years ago by teor

Replying to asn:

Yawning suggests this is a signed int overflow that leads to an abort because of ftrapv (#17983).

The overflow happens at:

  if (INT64_MAX - divisor + 1 < number)
    return INT64_MAX;

whose left side probably gets applied as INT64_MAX + 1 - divisor.

Optimising compilers FTW.

A potential fix here would be to reorder that if statement to:

  if (INT64_MAX - number < divisor - 1)
    return INT64_MAX;

That will overflow if number is negative, and I'm pretty sure it's the wrong comparison.
Did you mean:

  if (INT64_MAX - divisor < number - 1)
    return INT64_MAX;

maybe with an additional check that divisor >= 1.

The function already does the equivalent: tor_assert(divisor > 0);

comment:4 Changed 3 years ago by teor

We'd also need to tor_assert(number >= INT64_MIN + 1);

But wait, the later statements would still invoke undefined behaviour, so the compiler would be free to reach back in time and remove our asserts!

comment:5 Changed 3 years ago by asn

Status: newneeds_review

Let's move this forward. Please see my branch bug19130 for the fix suggested by teor in comment:3 .

It changes the check to:

  if (INT64_MAX - divisor < number - 1)
    return INT64_MAX;

Are there additional check that need to be added?

Also, should we fix the other round_*_to_next_multiple_of() functions which have a similar pattern? They deal with unsigned integers where an overflow is not UB; but maybe we should not cause overflows on purpose anyway.

comment:6 Changed 3 years ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

number - 1 can underflow if number is INT64_MIN.
So we need to skip the overflow check in that case.

How about:

if (number > INT64_MIN)
  if (INT64_MAX - divisor < number - 1)
    return INT64_MAX;

Otherwise this looks good, and yes, it would be sensible to explicitly handle overflow and underflow, rather than leaving it to the whims of compilers.

comment:7 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

Let me suggest a simpler fix: Nothing actually uses this function in a sane way! It's only called twice, and when it's called, it's given an inputs that can't be negative.

How do we feel about bug19130_the_easy_way ?

comment:8 Changed 3 years ago by teor

Status: needs_reviewmerge_ready

Well, avoiding using signed integers is one way to avoid signed overflow.
And we can deal with the unsigned overflow in the other functions in another ticket, if asn wants to.

Let's get the easy way merged.

comment:9 Changed 3 years ago by cypherpunks

rounded_cells_seen is now uint64_t but rounded_onions_seen is still int64_t. Shouldn't the later also be uint64_t?

comment:10 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Fixed and applying to master.

comment:11 Changed 3 years ago by nickm

Actual Points: .3
Keywords: TorCoreTeam201605 added
Milestone: Tor: 0.2.9.x-final
Points: .3
Severity: NormalCritical
Sponsor: SponsorS-can

comment:12 Changed 3 years ago by arma

Resolution: fixed
Status: closedreopened

comment:13 Changed 3 years ago by arma

Owner: set to nickm
Status: reopenedassigned

comment:14 Changed 3 years ago by arma

Resolution: fixed
Status: assignedclosed

comment:15 Changed 15 months ago by teor

Parent ID: #23414

We need a working round_int64_to_next_multiple_of() for #23414.

Note: See TracTickets for help on using tickets.