Page MenuHomePhabricator

[secp256k1] scratch: add stack frame support

Authored by deadalnix on Jan 14 2020, 01:20.



This is a backport of libsecp256k1's PR523

Test Plan
ninja check-secp256k1

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Jan 14 2020, 01:20
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 01:20
Fabien requested changes to this revision.Jan 15 2020, 11:21
Fabien added a subscriber: Fabien.
Fabien added inline comments.
48 ↗(On Diff #15425)

Should be <= since scratch->frame is a counter and not an index ?

275 ↗(On Diff #15425)

secp256k1_scratch_allocate_frame doesn't take a boolean argument, this looks like a typo for CHECK(secp256k1_scratch_allocate_frame(scratch, 500, 1) == 1);

This revision now requires changes to proceed.Jan 15 2020, 11:21
deadalnix added inline comments.Jan 15 2020, 13:40
48 ↗(On Diff #15425)

This is as per the source material. This is correct if the frames are counted from 0; which they most likely are.

275 ↗(On Diff #15425)

Even though this isn't what's in the PR, this seems to be the intended test.

deadalnix updated this revision to Diff 15480.Jan 15 2020, 13:43

Update erroneous test case

Fabien added inline comments.Jan 15 2020, 13:45
48 ↗(On Diff #15425)

This would be inconsistent with, for example, then assertion in secp256k1_scratch_destroy above: VERIFY_CHECK(scratch->frame == 0);. If the frames are counted from 0, then there might still be an allocated frame when the scratch is destroyed, causing a memory leak.

deadalnix added inline comments.Jan 15 2020, 13:49
48 ↗(On Diff #15425)

You are correct. This is counting, but the code is correct anyways, because if we are going to allocate a new frame, then we want the frame count to be bellow the max, that that at the end of the process we reach at most the max number of frames.

Fabien accepted this revision.Jan 15 2020, 13:51
This revision is now accepted and ready to land.Jan 15 2020, 13:51
This revision was automatically updated to reflect the committed changes.