Page MenuHomePhabricator

Merge #11411: script: Change SignatureHash input index check to an assert.
Changes PlannedPublic

Authored by nakihito on Thu, Aug 22, 00:12.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

5ddf560 script: Change SignatureHash input index check to an assert. (Jim Posen)

Pull request description:

In the SignatureHash function, the input index must refer to a valid index. This is not enforced equally in the segwit/non-segwit branches and should be an assertion rather than returning a error hash.

Tree-SHA512: a775fc9e9bd9835c0ab36368aa45ab3d53e38f31fd4d2b8684a77ee6028c854c363df038681a59358b30138957573ad63b71d4a722c16e0830fb4fa72669ef39

Backport of Core PR11411
https://github.com/bitcoin/bitcoin/pull/11411/

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11411
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7223
Build 12491: Bitcoin ABC Teamcity Staging
Build 12490: arc lint + arc unit

Event Timeline

nakihito created this revision.Thu, Aug 22, 00:12
Owners added a reviewer: Restricted Owners Package.Thu, Aug 22, 00:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Aug 22, 00:12
deadalnix requested changes to this revision.Thu, Aug 22, 18:15

What steps did you take to make sure that this assert is unreachable?

Also, why isn't it at the start of the function like in the PR you backported from.

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

What steps did you take to make sure that this assert is unreachable?
Also, why isn't it at the start of the function like in the PR you backported from.

I'm pretty sure that if nIn >= txTo.vin.size() would cause out of bounds errors? This is even mentioned on what was previously line 1511. Is make check not enough? It runs sighash_tests which I believe tests this. All this change does is cause a crash when there is invalid input rather than return one.

I did not put it at the top of the function because I was unsure if the code above it was from an out-of-order backport or not. Instead, I tried to place it relative to whatever the next line of code should have been. Looking at it now, I should have placed at the top of the function.

nakihito planned changes to this revision.Wed, Sep 11, 20:43