Opened 5 months ago
Closed 4 months ago
#29527 closed defect (fixed)
Division by zero: undefined behaviour in circuitpadding/circuitpadding_sample_distribution test
Reported by: | teor | Owned by: | teor |
---|---|---|---|
Priority: | High | Milestone: | Tor: 0.4.0.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | was-merged-to-041-after-29298, nickm-merge, teor-merge, regression, tor-ci, tor-test, 040-must, 040-backport, asn-merge |
Cc: | mikeperry, asn, riastradh | Actual Points: | |
Parent ID: | Points: | ||
Reviewer: | nickm | Sponsor: | Sponsor2-can |
Description
When running the tor unit tests on macOS with --enable-expensive-hardening
, I get the following error:
circuitpadding/circuitpadding_sample_distribution: [forking] ../src/lib/math/prob_distr.c:1311:17: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:1311:17 in ../src/lib/math/prob_distr.c:1219:49: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:1219:49 in OK
My full configure command-line is:
configure --disable-asciidoc --with-libevent-dir=/usr/local --with-openssl-dir=/usr/local/opt/openssl --enable-lzma --enable-zstd --enable-libscrypt CC=clang --enable-gcc-warnings --enable-expensive-hardening PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/local/share/pkgconfig
Child Tickets
Change History (34)
comment:1 Changed 5 months ago by
Keywords: | tor-test added |
---|
comment:2 Changed 5 months ago by
There are actually more of these than I thought:
prob_distr/logit_logistics: [forking] ../src/lib/math/prob_distr.c:427:26: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:427:26 in ../src/lib/math/prob_distr.c:344:17: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:344:17 in OK prob_distr/log_logistic: [forking] ../src/test/test_prob_distr.c:496:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:496:5 in ../src/test/test_prob_distr.c:496:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:496:5 in ../src/test/test_prob_distr.c:497:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:497:5 in ../src/test/test_prob_distr.c:497:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:497:5 in ../src/test/test_prob_distr.c:498:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:498:5 in ../src/test/test_prob_distr.c:498:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:498:5 in ../src/test/test_prob_distr.c:499:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:499:5 in ../src/test/test_prob_distr.c:499:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:499:5 in ../src/test/test_prob_distr.c:500:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:500:5 in ../src/test/test_prob_distr.c:500:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:500:5 in ../src/test/test_prob_distr.c:501:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:501:5 in ../src/test/test_prob_distr.c:501:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:501:5 in ../src/test/test_prob_distr.c:508:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:508:5 in ../src/test/test_prob_distr.c:508:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:508:5 in ../src/test/test_prob_distr.c:509:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:509:5 in ../src/test/test_prob_distr.c:509:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:509:5 in ../src/test/test_prob_distr.c:510:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:510:5 in ../src/test/test_prob_distr.c:510:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:510:5 in ../src/test/test_prob_distr.c:511:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:511:5 in ../src/test/test_prob_distr.c:511:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:511:5 in ../src/test/test_prob_distr.c:512:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:512:5 in ../src/test/test_prob_distr.c:512:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:512:5 in ../src/test/test_prob_distr.c:513:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:513:5 in ../src/test/test_prob_distr.c:513:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:513:5 in ../src/test/test_prob_distr.c:553:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:553:5 in ../src/lib/math/prob_distr.c:685:27: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:685:27 in ../src/test/test_prob_distr.c:553:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:553:5 in ../src/test/test_prob_distr.c:554:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:554:5 in ../src/test/test_prob_distr.c:554:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:554:5 in ../src/test/test_prob_distr.c:555:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:555:5 in ../src/test/test_prob_distr.c:555:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:555:5 in ../src/test/test_prob_distr.c:556:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:556:5 in ../src/test/test_prob_distr.c:556:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:556:5 in ../src/test/test_prob_distr.c:557:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:557:5 in ../src/test/test_prob_distr.c:557:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:557:5 in ../src/test/test_prob_distr.c:558:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:558:5 in ../src/test/test_prob_distr.c:558:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:558:5 in ../src/test/test_prob_distr.c:559:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:559:5 in ../src/test/test_prob_distr.c:559:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:559:5 in ../src/test/test_prob_distr.c:560:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:560:5 in ../src/test/test_prob_distr.c:560:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:560:5 in ../src/test/test_prob_distr.c:561:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:561:5 in ../src/test/test_prob_distr.c:561:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:561:5 in ../src/test/test_prob_distr.c:568:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:568:5 in ../src/lib/math/prob_distr.c:1170:20: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:1170:20 in ../src/test/test_prob_distr.c:568:5: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/test/test_prob_distr.c:568:5 in OK prob_distr/weibull: [forking] OK prob_distr/genpareto: [forking] ../src/lib/math/prob_distr.c:814:23: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:814:23 in ../src/lib/math/prob_distr.c:837:23: runtime error: division by zero SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/lib/math/prob_distr.c:837:23 in OK
comment:3 Changed 5 months ago by
Cc: | riastradh added |
---|
comment:4 Changed 5 months ago by
Priority: | Medium → High |
---|
comment:5 Changed 5 months ago by
For me this happens on linux too, with clang and --enable-fragile-hardening
comment:6 Changed 5 months ago by
Keywords: | 040-must added |
---|
Marking tickets as 040-must based on triage with dgoulet.
comment:7 follow-up: 9 Changed 5 months ago by
Floating-point division by zero isn't undefined behaviour -- it is precisely defined in IEEE 754 and has been consistently implemented in essentially all software and hardware for decades. This looks like mostly, if not entirely, false positives from a buggy UB sanitizer that's confused about floating-point arithmetic.
All of the cases in test_prob_distr.c appear to be working as intended. For example, test_prob_distr.c line 496 invokes CHECK_RELERR(np, cdf_log_logistic(1/x, 1, 1))
. The log-logistic distribution has the property that CDF(1/x) = 1 - CDF(x). When x is arbitrarily close to 0, 1 - CDF(x) should be extremely close to 1; when we round x to zero, CDF(x) should be 0, 1/x is rounded to infinity, and CDF(1/x) = 1 - CDF(x) should be 1, exactly as the test confirms.
In prob_distr.c:
- line 344, column 17 is inside logit. logit(1) should be +inf (lim_{x --> 1^{-}} logit(x) = +inf), and the +inf yielded by division by zero is correctly propagated by the expression log(p/(1 - p)). The test suite specifically checks that logit(1) = +inf (test_prob_distr.c lines 323 and 395).
- line 427, column 26 is inside logithalf. logithalf(1/2) = logit(1) should be +inf, and the +inf yielded by division by zero is correctly propagated by the expression log((0.5 + p0)/(0.5 - p0)). The test suite specifically checks that logithalf(+/-1/2) = +/-inf (test_prob_distr.c lines 294, 323, and 397).
- line 685, column 27 is inside isf_log_logistic. The log logistic distribution has the property that CDF(1/x) = 1 - CDF(x), which means CDF^{-1}(1 - p) = SF^{-1}(p) = 1/x. In test_prob_distr.c, line 450 specifies a test where p = 0, np = 1 - p = 1, and line 553 confirms isf_log_logistic(p, 1, 1), which entails dividing by p, correctly gives 1/x.
- line 814, column 23 is in cdf_genpareto. Here we are checking whether the approximation 1 - e^{-x_0} is safe for the specified value of xi, which we consider it to be if |xi| < 1e-17/x_0. If x_0 is infinite, we consider it safe. For any xi, as x_0 goes to zero, the CDF goes to zero too, which is exactly what the approximation computes, so this is correct. The test case on line 718 of test_prob_distr.c specifies x_0 = 0. (Side note: It appears I wrote no tests with a nonzero mean. Should maybe add some.)
- line 837, column 23 is in sf_genpareto, which has the same case analysis and test cases as the above instance.
- line 1170, column 20 is in sample_log_logistic, evaluating the quotient in
(1 - p0)/p0
. This is division by zero only if p0 = 0. In that case, we return the correct answer is +inf, since as p0 -> 0, this function grows without bound. In test_prob_distr.c, lines 565, 567, and 569 specify tests with p0 = 0. (However, in actual runs of the code with p0 drawn usingrandom_uniform_01
, p0 = 0 should happen only with probability 2^{-1075}, i.e. never.) - line 1311, column 17 is inside sample_geometric, evaluating the quotient in
-x/log1p(-p)
. The divisor can be zero only if p = 0, which means we're trying to sample from a geometric distribution with zero success probability. Of course, the only reasonable result is +inf. Is there anything _upstream_ of this logic that tries to construct aCIRCPAD_DIST_GEOMETRIC
with zero success probability? - line 1219, column 49 is inside sample_weibull, computing
1/k
. This can happen only if k = 0, which is an edge case that's probably not very useful, but the expression does return +inf which is the correct limit as k --> 0. Is there anything upstream of this logic that tries to construct aCIRCPAD_DIST_WEIBULL
with zero shape?
By the way, pretty much all non-arm hardware supports a kind of ‘sanitizer’ for floating-point invalid operations (which are not undefined behaviour, but are usually undesirable) with zero overhead if they don't happen, namely trapping the invalid-operation exception, which Unix will translate to SIGFPE. (Most arm hardware supports the exception, but only via sticky bits you can explicitly test with fetestexcept
, not via trapping.)
I tried running the tests with tinytest.c modified to do feenableexcept(FE_INVALID)
first thing in tinytest_main (needs #define _GNU_SOURCE
and #include <fenv.h>
). This turned up only one invalid operation in the tests: the logsumexp in test_stochastic_geometric_impl slightly exceeds zero, so log1mexp performs an invalid operation (log of negative); it is safe to replace log1mexp(logsumexp(...))
here by log1mexp(fmin(0, logsumexp(...)))
to avoid this.
(The issue in test_stochastic_geometric_impl does not invalidate any of the test results: the NaN it produced without exceptions trapped was never used again in the subsequent computation, because it should have been an effectively zero probability (less than e^{-100} = 2^{-144} or something) and the corresponding count is always zero in tests as it should be, so psi_test ignores the probability. If the count were ever nonzero, indicating a broken geometric sampler, psi would be computed as a NaN and the psi test would fail. So floating-point arithmetic once again does the right thing in the end, though it still would probably be better to use log1mexp(fmin(0, logsumexp(...)))
.)
comment:8 Changed 5 months ago by
test_circuitpadding.c, helper_circpad_circ_distribution_machine_setup has the following comment:
/** Helper function: Initializes a padding machine where every state uses the * uniform probability distribution. */
However, it proceeds to set up several other distributions with nonsensical and/or ignored parameters:
zero_st->iat_dist.type = CIRCPAD_DIST_UNIFORM; zero_st->iat_dist.param1 = min; zero_st->iat_dist.param2 = max; ... first_st->iat_dist.type = CIRCPAD_DIST_LOGISTIC; first_st->iat_dist.param1 = min; first_st->iat_dist.param2 = max; ... second_st->iat_dist.type = CIRCPAD_DIST_LOG_LOGISTIC; second_st->iat_dist.param1 = min; second_st->iat_dist.param2 = max; ... third_st->iat_dist.type = CIRCPAD_DIST_GEOMETRIC; third_st->iat_dist.param1 = min; third_st->iat_dist.param2 = max; ... fourth_st->iat_dist.type = CIRCPAD_DIST_WEIBULL; fourth_st->iat_dist.param1 = min; fourth_st->iat_dist.param2 = max; ... fifth_st->iat_dist.type = CIRCPAD_DIST_PARETO; fifth_st->iat_dist.param1 = min; fifth_st->iat_dist.param2 = max;
Is this intended? (Here min=0 and max=10 in the solitary call site, which might explain the geometric and Weibull distributions with zero parameters.)
comment:9 Changed 5 months ago by
Replying to riastradh:
...
I tried running the tests with tinytest.c modified to do
feenableexcept(FE_INVALID)
first thing in tinytest_main (needs#define _GNU_SOURCE
and#include <fenv.h>
). This turned up only one invalid operation in the tests: the logsumexp in test_stochastic_geometric_impl slightly exceeds zero, so log1mexp performs an invalid operation (log of negative); it is safe to replacelog1mexp(logsumexp(...))
here bylog1mexp(fmin(0, logsumexp(...)))
to avoid this.
(The issue in test_stochastic_geometric_impl does not invalidate any of the test results: the NaN it produced without exceptions trapped was never used again in the subsequent computation, because it should have been an effectively zero probability (less than e^{-100} = 2^{-144} or something) and the corresponding count is always zero in tests as it should be, so psi_test ignores the probability. If the count were ever nonzero, indicating a broken geometric sampler, psi would be computed as a NaN and the psi test would fail. So floating-point arithmetic once again does the right thing in the end, though it still would probably be better to use
log1mexp(fmin(0, logsumexp(...)))
.)
Ok, let's fix these issues in this ticket?
#29528 deals with the general case.
comment:10 Changed 5 months ago by
From https://trac.torproject.org/projects/tor/ticket/29528#comment:3
There is an open Clang bug for this: https://bugs.llvm.org/show_bug.cgi?id=19535
Possible workaround: use
-fno-sanitize=float-divide-by-zero
in addition to-fsanitize=undefined
.
Let's also implement this workaround for the warnings that are obviously not bugs.
comment:11 follow-up: 13 Changed 5 months ago by
Status: | new → needs_review |
---|
Thanks for the great analysis in this ticket. Based on the comments here it was quite easy to to write a patch.
You can see it in https://github.com/torproject/tor/pull/722
Please note that only the last two commits are about this ticket, the others are there because I based the branch on #29298 to avoid some conflicts in the future.
The first commit (a64ccf151f) fixes the probability distribution parameters that Riastradh mentioned with bold in comment:7. The second commit (f708055e9f) silences float-divide-by-zero as suggested by #29528.
comment:12 Changed 5 months ago by
Reviewer: | → mikeperry |
---|
comment:13 follow-up: 14 Changed 5 months ago by
Replying to asn:
The first commit (a64ccf151f) fixes the probability distribution parameters that Riastradh mentioned with bold in comment:7. The second commit (f708055e9f) silences float-divide-by-zero as suggested by #29528.
Cool. One suggestion for a change and one comment:
- It might be helpful if you put a comment on each line describing what the name and purpose of the parameter is, like
/* k, shape parameter */
, because I can never remember which one goes first and which one goes last.
- This is the kind of code where the
struct weibull dist = { WEIBULL(dist), .k = ..., .lambda = ... }
can be much more legible thanstruct circpad_distribution dist = { .type = CIRCPAD_DIST_WEIBULL, .param1 = ..., .param2 = ... }
. (Obviously changing the code to work like that here is more involved than would be warranted, even if you wanted it, for this particular issue, of course.)
comment:14 Changed 5 months ago by
Replying to riastradh:
Replying to asn:
The first commit (a64ccf151f) fixes the probability distribution parameters that Riastradh mentioned with bold in comment:7. The second commit (f708055e9f) silences float-divide-by-zero as suggested by #29528.
Cool. One suggestion for a change and one comment:
- It might be helpful if you put a comment on each line describing what the name and purpose of the parameter is, like
/* k, shape parameter */
, because I can never remember which one goes first and which one goes last.
Agreed! I pushed a fixup that tries to do this: https://github.com/torproject/tor/pull/722/commits/5b616a4831979bf25ad3429b0673c00f2757dd25
- This is the kind of code where the
struct weibull dist = { WEIBULL(dist), .k = ..., .lambda = ... }
can be much more legible thanstruct circpad_distribution dist = { .type = CIRCPAD_DIST_WEIBULL, .param1 = ..., .param2 = ... }
. (Obviously changing the code to work like that here is more involved than would be warranted, even if you wanted it, for this particular issue, of course.)
Agreed. Let's not try to do this in this bugfix ticket but we can probably do it in 041 as part of #28636: https://trac.torproject.org/projects/tor/ticket/28636#comment:13
comment:15 Changed 5 months ago by
Status: | needs_review → merge_ready |
---|
Little nervous about disabling divide by zero sanitation across all of Tor for this code, but it's not actually undefined behavior, so it's probably ok.
Thought I should still just flag that for Nick in case he wants to hack autoconf to apply -fno-sanitize=float-divide-by-zero to just this c file somehow.
comment:16 follow-up: 17 Changed 5 months ago by
I'm fine with -fno-sanitize=float-divide-by-zero
globally. Should I wait until #29298 is merged to merge this branch?
comment:17 Changed 5 months ago by
Replying to nickm:
I'm fine with
-fno-sanitize=float-divide-by-zero
globally. Should I wait until #29298 is merged to merge this branch?
That'd be cool and make things easier if possible.
Otherwise, let me know and I can make a branch based on master (but then I would need to make a different branch for #29298).
comment:18 Changed 4 months ago by
Sponsor: | → Sponsor2-can |
---|
comment:19 Changed 4 months ago by
Keywords: | nickm-merge teor-merge added |
---|
comment:20 Changed 4 months ago by
Keywords: | merge-after-29298 added |
---|
comment:21 Changed 4 months ago by
Parent ID: | → #29298 |
---|
We don't really have a "waiting for another ticket" status.
comment:22 Changed 4 months ago by
Status: | merge_ready → needs_information |
---|
comment:23 Changed 4 months ago by
Resolution: | → fixed |
---|---|
Status: | needs_information → closed |
Squashed and merged!
comment:24 follow-up: 25 Changed 4 months ago by
Parent ID: | #29298 |
---|
(Does this need a (partial?) backport to 0.4.0? Please reopen if so.)
comment:25 follow-up: 26 Changed 4 months ago by
comment:26 Changed 4 months ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Replying to asn:
Replying to nickm:
(Does this need a (partial?) backport to 0.4.0? Please reopen if so.)
The issues are there on 040 but perhaps we dont need to fix them since they dont fail the build or the CI, except if we also merge #29528 to 040.
I think we should backport, otherwise users will notice and send us bug reports.
Also, there are no guarantees that future UndefinedBehaviourSanitizers will continue on error.
I guess that means we need a cherry-pick to 0.4.0?
comment:27 Changed 4 months ago by
Keywords: | 040-backport added |
---|
comment:28 Changed 4 months ago by
Owner: | set to teor |
---|---|
Status: | reopened → assigned |
I can take ownership of this backport,
comment:29 Changed 4 months ago by
Status: | assigned → needs_revision |
---|
We need to backport the minimal change to 0.4.0, then merge.
comment:30 Changed 4 months ago by
Keywords: | was-merged-to-041-after-29298 added; merge-after-29298 removed |
---|---|
Status: | needs_revision → needs_review |
See my pull request:
https://github.com/torproject/tor/pull/811
This branch merges cleanly to master, because the cherry-picked commit f708055 is already in master. (So it just adds a changes file, which contains useful information in addition to the 0.4.1 changes file.)
asn, this ticket had two commits in 0.4.1:
- Fix test prob distr parameters that caused warnings. a64ccf1
- Silence unneeded clang warns that triggered in prob distr tests. f708055
https://github.com/torproject/tor/pull/722
But I only backported the final one (f708055).
a64ccf1 didn't apply cleanly to maint-0.4.0, which means the tests will diverge slightly. I hope that doesn't affect the stochastic failure rate that much in #29693.
comment:31 Changed 4 months ago by
Reviewer: | mikeperry |
---|
Remove Mike as reviewer, because he's overloaded.
We'll work out what to do with these tickets in the weekly meeting.
comment:32 Changed 4 months ago by
Reviewer: | → nickm |
---|
comment:33 Changed 4 months ago by
Keywords: | asn-merge added |
---|---|
Status: | needs_review → merge_ready |
LGTM; let's merge to 0.4.0.
I opened #29528 to fix the general case: we should fail on sanitizer errors.