Page MenuHomePhabricator

Fix a potential use after move
ClosedPublic

Authored by Fabien on Thu, Oct 15, 19:18.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC59239db2aa64: Fix a potential use after move
Summary

When checking the tx inputs with CheckInputs(), the signature check
count is expected to be 0 when pvChecks is non null. This diff fixes
the current implementation which used the check variable after a move
to increment the sig check count with 0.

Test Plan
ninja all check-all

Diff Detail

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

Event Timeline

Fabien requested review of this revision.Thu, Oct 15, 19:19
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/validation.cpp
1074 ↗(On Diff #24679)

Remove the else.

This revision is now accepted and ready to land.Thu, Oct 15, 21:34
deadalnix requested changes to this revision.Thu, Oct 15, 21:45
deadalnix added inline comments.
src/validation.cpp
1074 ↗(On Diff #24679)

And add a comment about what is going on.

This revision now requires changes to proceed.Thu, Oct 15, 21:45
deadalnix requested changes to this revision.Thu, Oct 15, 21:52
deadalnix added inline comments.
src/validation.cpp
1076 ↗(On Diff #24685)

You need to add a comment explaining the flow here because it is confusing.

This revision now requires changes to proceed.Thu, Oct 15, 21:52

Attempt to explain the flow

deadalnix requested changes to this revision.Thu, Oct 15, 22:22
deadalnix added inline comments.
src/validation.cpp
1073 ↗(On Diff #24689)

The sigcheck will definitively be counted. his comment is at best misleading.

1079 ↗(On Diff #24689)

Anyone who ends up here can read the code. Repeating the code in English doesn't help. There is a reason WHY the code is structured this way. This is what you need to explain.

This revision now requires changes to proceed.Thu, Oct 15, 22:22

Simplify comments and explain the intent rather than the code itself

This revision is now accepted and ready to land.Fri, Oct 16, 13:15
This revision was automatically updated to reflect the committed changes.