Opened 5 years ago

Closed 5 years ago

#17827 closed defect (fixed)

Different return type of backtrace function on FreeBSD

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.2-alpha
Severity: Normal Keywords: 027-backport freebsd easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Jenkins is failing compilation on FreeBSD because its backtrace function is returning size_t instead of int. This leads to the compilation errors such as

../tor/src/common/backtrace.c:98:11: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
  depth = backtrace(cb_buf, MAX_DEPTH);
        ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../tor/src/common/backtrace.c:130:11: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
  depth = backtrace(cb_buf, MAX_DEPTH);
        ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../tor/src/common/backtrace.c:177:17: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
    int depth = backtrace(cb_buf, MAX_DEPTH);
        ~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Child Tickets

Attachments (1)

0001-Fix-backtrace-compilation-on-FreeBSD.patch (3.5 KB) - added by cypherpunks 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by teor

Milestone: Tor: 0.2.8.x-final

We can probably just use a cast to int to avoid this issue, as long as we're sure the return value fits in an int.

What's the range of the return value?

comment:2 in reply to:  1 Changed 5 years ago by nickm

Keywords: 027-backport freebsd easy added

Replying to teor:

We can probably just use a cast to int to avoid this issue, as long as we're sure the return value fits in an int.

What's the range of the return value?

The Linux manpage says

       int backtrace(void **buffer, int size);
....
       backtrace() returns the number of addresses returned in  buffer,  which
       is  not greater than size.

FreeBSD says

     size_t
     backtrace(void **addrlist,	size_t len);
...
     The backtrace() function returns the number of elements that were filled
     in	the backtrace.

So either way, if we pass something less than INT_MAX in, we get something less than int_max out.

comment:3 Changed 5 years ago by cypherpunks

The FreeBSD versions of backtrace_symbols and backtrace_symbols_fd also expect their len parameter to be size_t. So we need to make sure depth is larger than zero to prevent overflow issues.

comment:4 in reply to:  3 Changed 5 years ago by teor

Replying to cypherpunks:

The FreeBSD versions of backtrace_symbols and backtrace_symbols_fd also expect their len parameter to be size_t. So we need to make sure depth is larger than zero to prevent overflow issues.

We already set MAX_DEPTH to 256, so anything outside the range 0 <= len <= MAX_DEPTH is an error. (If that happens, we should probably assert, or return from the function.)

This means that we could use size_t (or long) for len instead of int. Then we'd have no shortening warnings.

We could also change the type of depth in the clean_backtrace arguments, and therefore the type of depth in sigsys_debugging.

I think this is the best way to patch this issue, rather than casting - it expresses the intended range of depth / len more clearly than a cast.

But I'd also happily take a patch that used casts to silence the warnings.

Changed 5 years ago by cypherpunks

comment:5 Changed 5 years ago by cypherpunks

Status: newneeds_review
Version: Tor: 0.2.5.2-alpha

comment:6 Changed 5 years ago by teor

Looks great, let's get this merged!

Why this patch works:

  • int is smaller than or equal to size_t on all platforms, so when we cast int to size_t, clang won't give any warnings.
  • clang doesn't give any warnings when a size_t is passed to the function, at least on OS X (x86_64 & i386) and Linux (x86_64) where I tested it.

Why this patch works on the OSs not mentioned above:

  • OS X also uses int, so the function signatures are the same as Linux.

Open questions:

  • Do we do backtraces on Windows? (I can't find a Windows-specific backtrace function to check the argument types.)

comment:7 Changed 5 years ago by nickm

Thanks; merged it.

Do we do backtraces on Windows? (I can't find a Windows-specific backtrace function to check the argument types.)

No; the windows code we'd need is completely different. Windows hackers should see #10186 for progress there.

comment:8 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:9 Changed 5 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

It seems compilation is now failing on Linux. When we use a size_t input for backtrace_symbols and backtrace_symbols_fd on Linux the integer precision is implicitly decreased by stuffing it in the int parameter of these functions. This leads to the following errors.

src/common/backtrace.c:99:39: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
  symbols = backtrace_symbols(cb_buf, depth);
            ~~~~~~~~~~~~~~~~~         ^~~~~
src/common/backtrace.c:142:34: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
    backtrace_symbols_fd(cb_buf, depth, fds[i]);
    ~~~~~~~~~~~~~~~~~~~~         ^~~~~
src/common/backtrace.c:178:41: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
    symbols = backtrace_symbols(cb_buf, depth);
              ~~~~~~~~~~~~~~~~~         ^~~~~
3 errors generated.

We can solve these errors by either casting the depth parameter to int (which would then be implicitly cast back to size_t on FreeBSD) or use preprocessor checks to account for the differences between Linux and FreeBSD.

My preference is the later solution because it looks cleaner IMO and we already do such checks in clean_backtrace to handle stack manipulation on different platforms. What does anybody else think?

comment:10 Changed 5 years ago by nickm

Resolution: fixed
Status: reopenedclosed

I think the cast is fine in this case; added it as c4df0c9.

Thanks!

comment:11 Changed 5 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

I forgot to mention that the sandbox code also contains an instance of backtrace_symbols_fd which has the same issue.

Can you also backport these fixes like you did with the original patch?

comment:12 Changed 5 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Done and done. Thanks!

Note: See TracTickets for help on using tickets.