The obvious fix for #300. I'm not sure if this should be handled in the same macro though.
bugfix: don't blow up on "ARG_CHECK(ctx != NULL)" #301
pull theuni wants to merge 1 commits into bitcoin-core:master from theuni:arg_check changing 3 files +17 −17-
theuni commented at 6:21 PM on September 1, 2015: contributor
-
apoelstra commented at 6:36 PM on September 1, 2015: contributor
I think a better fix is to replace the affected
ARG_CHECKs withVERIFY_CHECK. Or even to put aVERIFY_CHECKforctx != NULLin theARG_CHECKmacro itself.I think we'll get burned if we don't terminate the program on null context, since it's used inside of so many macros.
-
sipa commented at 7:09 PM on September 1, 2015: contributor
Agree with @apoelstra. This will just make the accidental passing of ctx==NULL harder to discover. On Sep 1, 2015 20:36, "Andrew Poelstra" notifications@github.com wrote:
I think a better fix is to replace the affected ARG_CHECKs with VERIFY_CHECK. Or even to put a VERIFY_CHECK for ctx != NULL in the ARG_CHECK macro itself.
I think we'll get burned if we don't terminate the program on null context, since it's used inside of so many macros.
— Reply to this email directly or view it on GitHub https://github.com/bitcoin/secp256k1/pull/301#issuecomment-136822520.
-
theuni commented at 7:11 PM on September 1, 2015: contributor
Ok, fine by me. This was just the "fix it as it lies" change. I'd much prefer to handle the ctx logic separately from the other args.
-
apoelstra commented at 5:51 PM on September 2, 2015: contributor
Can you replace this with a patch that replaces any
ARG_CHECKs onctxwithVERIFY_CHECK? (If not I can do it.) - theuni force-pushed on Sep 3, 2015
- theuni force-pushed on Sep 3, 2015
-
theuni commented at 2:08 AM on September 3, 2015: contributor
Updated as suggested. Also moved one case where the context was checked after other arguments.
- theuni force-pushed on Sep 3, 2015
-
theuni commented at 3:40 AM on September 3, 2015: contributor
Updated again. I accidentally left the original change to the ARG_CHECK macro in, gone now.
-
b183b41122
bugfix: "ARG_CHECK(ctx != NULL)" makes no sense
Move all context checks to VERIFY_CHECK and be sure they come before all ARG_CHECKs.
- sipa merged this on Sep 4, 2015
- sipa closed this on Sep 4, 2015
- sipa referenced this in commit c822693eff on Sep 4, 2015
- sipa cross-referenced this on Sep 29, 2015 from issue Bad callback logic by theuni
Linked (view graph)