tests: Abort if malloc() fails during context cloning tests #774

real-or-random commented at 11:49 AM on July 27, 2020: contributor

Found by the clang static analyzer.

This is the worst true positive that it found. I feel somewhat proud.

real-or-random commented at 11:53 AM on July 27, 2020: contributor

By the way, the static analyzer is very easy to run and it's very easy to inspect the results: https://clang-analyzer.llvm.org/scan-build.html You need clang 10.

I run this with internal bignum, asm disabled (you get more false positives otherwise) on 64 bit without endo. Other configurations may be interesting, too, and I invite everyone to verify that the remaining issues are false positives.

gmaxwell commented at 6:08 PM on July 27, 2020: contributor

I was going to comment that the cloning blocks seemed misplaced in the middle of error counting tests, but I see why they were added there: to check if clone copies the error callback. Reasonable enough change.

For extra fun, it's possible to actually test handling of malloc failure: https://github.com/xiph/opus/blob/master/tests/test_opus_api.c#L1751

in src/tests.c:185 in 3b789d5edb outdated

 181 | @@ -182,7 +182,8 @@ void run_context_tests(int use_prealloc) {
 182 |      ecount2 = 10;
 183 |      secp256k1_context_set_illegal_callback(vrfy, counting_illegal_callback_fn, &ecount);
 184 |      secp256k1_context_set_illegal_callback(sign, counting_illegal_callback_fn, &ecount2);
 185 | -    secp256k1_context_set_error_callback(sign, counting_illegal_callback_fn, NULL);
 186 | +    /* try to set error callback (to a function that still aborts in case malloc() fails in secp256k1_context_clone() below) */

jonasnick commented at 6:59 PM on July 27, 2020:

nit: "try"?

real-or-random commented at 8:25 AM on July 28, 2020:

Yeah, try and see if it works. It's all over that function: https://github.com/bitcoin-core/secp256k1/blob/3b789d5edbf9faa356744d71ddd023ecf076ec90/src/tests.c#L303

But okay, I've removed it now and also made the checks more precise.

jonasnick commented at 7:02 PM on July 27, 2020: contributor

ACK mod nit

Fwiw, I've used scan-build a couple of times on this code base over the years so I doubt it needs clang 10.

tests: Abort if malloc() fails during context cloning tests

Found by the clang static analyzer.

This is the worst true positive that it found. I feel somewhat proud.

2e1b9e0458

real-or-random force-pushed on Jul 28, 2020

elichai commented at 8:26 AM on July 28, 2020: contributor

I run this with internal bignum, asm disabled (you get more false positives otherwise) on 64 bit without endo. Other configurations may be interesting, too, and I invite everyone to verify that the remaining issues are false positives.

Ha, that's interesting, I was spending half an hour trying to understand the 42 "bugs" it found and they didn't make any sense. disabling asm does removes most of them(when will clang tools will start understanding asm :( )

real-or-random commented at 8:27 AM on July 28, 2020: contributor

Fwiw, I've used scan-build a couple of times on this code base over the years so I doubt it needs clang 10.

You're right, and I was somewhat confused because I remember you showed me output of a toolthat looked very similar a while ago. I was totally confusing this with GCC 10, which has a new analyzer since v10 (-fanalyzer)... Anyway.

real-or-random commented at 8:29 AM on July 28, 2020: contributor

Ha, that's interesting, I was spending half an hour trying to understand the 42 "bugs" it found and they didn't make any sense. disabling asm does removes most of them(when will clang tools will start understanding asm :( )

Even with asm disabled, it shows 20 positive for me after the PR, which I all looked at manually. So it could be interesting to look at all 42 ones and see if they are really false positives.

elichai commented at 8:31 AM on July 28, 2020: contributor

Ha, that's interesting, I was spending half an hour trying to understand the 42 "bugs" it found and they didn't make any sense. disabling asm does removes most of them(when will clang tools will start understanding asm :( )

Even with asm disabled, it shows 20 positive for me after the PR. So it could be interesting to look at all 42 ones and see if they are really false positives.

yep, I'm trying to go over them right now (some are just we didn't check malloc returning NULL in the tests)

elichai commented at 8:45 AM on July 28, 2020: contributor

tACK 2e1b9e0458317d03b682c1f5dd63aedb52c86b04 Can confirm that this PR made the following line disappear(in scan-view): Logic error | Dereference of null pointer | tests.c | counting_illegal_callback_fn | 43 | 82 Also the change makes sense (the alternative would be to check that the count wasn't incremented after each malloc)

in src/tests.c:186 in 2e1b9e0458

 181 | @@ -182,8 +182,10 @@ void run_context_tests(int use_prealloc) {
 182 |      ecount2 = 10;
 183 |      secp256k1_context_set_illegal_callback(vrfy, counting_illegal_callback_fn, &ecount);
 184 |      secp256k1_context_set_illegal_callback(sign, counting_illegal_callback_fn, &ecount2);
 185 | -    secp256k1_context_set_error_callback(sign, counting_illegal_callback_fn, NULL);
 186 | -    CHECK(vrfy->error_callback.fn != sign->error_callback.fn);
 187 | +    /* set error callback (to a function that still aborts in case malloc() fails in secp256k1_context_clone() below) */
 188 | +    secp256k1_context_set_error_callback(sign, secp256k1_default_illegal_callback_fn, NULL);

elichai commented at 8:56 AM on July 28, 2020:

I see now that in other places we do secp256k1_context_set_error_callback(sign, NULL, NULL); instead of explicitly specifying the function

gmaxwell commented at 9:00 AM on July 28, 2020:

NULL sets it to the default.

elichai commented at 9:13 AM on July 28, 2020:

I missed the fact that this isn't actually the default function. but both will abort, so I think that was by mistake?

gmaxwell commented at 9:28 AM on July 28, 2020:

No, this is intentional. The purpose of the next line is to verify that the setter actually sets (and isn't a no-op). So it needs to be set to something different to perform that test.

Some additional tests added after the initial setter-test were introduced call the clone function so they actually need the context to be functional. They still logically should be using this context-- rather than getting their own which isn't saddled with some weird setter test--, however, because they're calling clone to verify (among other things) that the clone copies the configured setter function. If they had their own context they'd need the same weird setup to verify the clone copies the setting.

Your comments suggest that this is unclear enough that it deserves an explicit comment. /* This uses secp256k1_default_illegal_callback_fn instead of secp256k1_default_error_callback_fn because it needs to invoke it with non-default function as the argument for the following setter and clone tests, but the configured function should still work correctly should malloc happen to get called during the tests. */

elichai commented at 9:43 AM on July 28, 2020:

I see now that I keep getting confused between error and illegal :) your response makes sense. thanks.

real-or-random commented at 9:46 AM on July 28, 2020:

No three lines above it sets the illegal cb, not the error cb.

real-or-random commented at 10:02 AM on July 28, 2020:

so you stick to your ACK?

elichai commented at 10:22 AM on July 28, 2020:

yep. it was a nit anyway, and it turned out as a wrong nit :) (but feel free to add a comment similar to what @gmaxwell was suggesting)

real-or-random merged this on Jul 28, 2020

real-or-random closed this on Jul 28, 2020

jasonbcox referenced this in commit 50c8661a7b on Sep 27, 2020

deadalnix referenced this in commit 7c58c39faa on Sep 28, 2020