Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#14090 closed defect (fixed)

Test case "laplace" fails on Raspberry Debian 7.6

Reported by: TvdW Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.2-alpha
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

FAIL ../src/test/test_util.c:4667: assert(INT64_MIN + 20 == add_laplace_noise(20, 0.0, delta_f, epsilon))

[laplace FAILED]

This was reported on IRC, with a partial compilation log at http://pastebin.com/QgZM8VVQ

The only other thing I know is that this was reported for "Debian 7.6 wheezy on a raspberry pi"

Child Tickets

Change History (17)

comment:1 Changed 5 years ago by karsten

Can we get the reporter to compile and test teor's laplace-edge-cases branch and tell us how tests work out there? See also #13192 for a discussion of that branch.

comment:2 Changed 5 years ago by Device

I am the reporter. I cloned the laplace-edge-cases branch and compiled it
http://pastebin.com/kW7i8EzG is ./configure
http://pastebin.com/nT0hcRUy is the make
it seems to be compiling well, although it seems the git only has version 2.6.1 while I tried to compile 2.6.2

Jan 03 13:52:37.213 [notice] Tor v0.2.6.1-alpha-dev (git-b823b7aad83489ff) running on Linux with Libevent 2.0.19-stable, OpenSSL 1.0.1e and Zlib 1.2.7.

Version 0, edited 5 years ago by Device (next)

comment:3 Changed 5 years ago by karsten

What does make test say?

(And yes, that branch is based on 0.2.6.1, but it might fix the (apparently) broken unit test that is also contained in 0.2.6.2. The idea is that if this branch fixes your bug, we should rebase the patch or a variant of it to 0.2.6.2.)

comment:4 Changed 5 years ago by Device

This is the result from make test: http://pastebin.com/8SxWkrXu
looks like the bug is in this version as well, but I'll leave the conclusion to you.

comment:5 Changed 5 years ago by asn

After a discussion with Yawning and nickm, it seems that the cause of this is the following part of the C spec:

When a finite value of real floating type is converted to an integer
type other than _Bool, the fractional part is discarded (i.e., the
value is truncated toward zero). If the value of the integral part
cannot be represented by the integer type, the behavior is undefined

So basically, if sample_laplace_distribution() returns a double whose integral part is bigger than what int64_t can represent, then that value gets casted to int64_t and causes undefined behavior.

Nick suggested checking for big values like this in sample_laplace_distribution() in a fashion similar to this:

double result = mu - b * (p > 0.5 ? 1.0 : -1.0)
                       * tor_mathlog(1.0 - 2.0 * fabs(p - 0.5));
if (result >= INT64_MAX)
   return INT64_MAX;
else if (result <= INT64_MIN)
   return INT64_MIN;
else
   return (int64_t) result;

comment:6 Changed 5 years ago by asn

Milestone: Tor: 0.2.6.x-final
Status: newneeds_review

OK, I pushed a branch that addresses the undefined behavior spotted by Nick, as discussed in my previous comment. It can be found in branch bug14090 in my repository https://git.torproject.org/user/asn/tor.git. You can see the changes here: https://gitweb.torproject.org/user/asn/tor.git/commit/?h=bug14090&id=45bc5a07434376c0d801a547b38893d85a515b49 . Hopefully it's correct.

Device, could you please test out my branch, and run make test again to see if it fails? If it fails again, please pastebin the output like you so cleverly did before. Thank you very much!

PS: The branch includes an extra commit that uses tt_int_op() in the rest of the unittests, so that if this undefined behavior is not the problem here, we can see better the values compared.

PS2: BTW, I changed the sample_laplace_distribution() to return an int64_t to reduce the amount of casting happening in the code. We might want to rename that function to sample_laplace_distribution_int64() or something.

Last edited 5 years ago by asn (previous) (diff)

comment:7 Changed 5 years ago by nickm

needs another tweak: when you're comparing int64_t in a unit test, you need tt_i64_op instead of tt_int_op. Otherwise looks fine to me!

comment:8 in reply to:  7 Changed 5 years ago by asn

Replying to nickm:

needs another tweak: when you're comparing int64_t in a unit test, you need tt_i64_op instead of tt_int_op. Otherwise looks fine to me!

Fixed and pushed. Thanks for the notice.

comment:9 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

comment:10 Changed 5 years ago by asn

Builds on wheezy i386 are failing with:

../src/test/test_util.c: In function 'test_util_round_to_next_multiple_of':
../src/test/test_util.c:4635:3: error: overflow in implicit constant conversion [-Werror=overflow]
../src/test/test_util.c:4636:3: error: overflow in implicit constant conversion [-Werror=overflow]

This might be because I didn't convert all the tests to tt_i64_op. I pushed another commit that converts also the round_int64_to_next_multiple_of() tests. Please fetch my repo again.

comment:11 Changed 5 years ago by asn

Resolution: fixed
Status: closedreopened

Please see comment:10 for a build failure on i386.

comment:12 Changed 5 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Better now I hope?

comment:13 Changed 5 years ago by Device

asn I ran make test for your git and I got:
./configure: http://pastebin.com/vsJHcQU9
make test: http://pastebin.com/MRg8UdLL

Seems like there's no errors, only one skipped.

comment:14 Changed 5 years ago by karsten

Device, looks like that's not the branch you wanted: "Tor version is 0.2.4.0-alpha-dev". Can you try again with the latest official master branch?

comment:15 Changed 5 years ago by Device

asn asked me to run his personal git, which apparently is some old alpha.

I did a make test for the latest official master branch and got: http://pastebin.com/AEgbSRbG

comment:16 Changed 5 years ago by karsten

Looks good: util/laplace: OK

Thanks!

comment:17 Changed 5 years ago by asn

Thanks for testing, Device!

Note: See TracTickets for help on using tickets.