Fix oppushdata overflow bug

[caevv]

fix panic:
panic: runtime error: slice bounds out of range [3:2]

[codecov-commenter]

Codecov Report

Merging #22 (a279d95) into master (e5e01aa) will decrease coverage by 2.39%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   78.28%   75.88%   -2.40%     
==========================================
  Files          16       16              
  Lines        1165      846     -319     
==========================================
- Hits          912      642     -270     
+ Misses        166      140      -26     
+ Partials       87       64      -23     

Impacted Files	Coverage Δ
bscript/oppushdata.go	65.27% <0.00%> (ø)
internalsigner.go	61.29% <0.00%> (-24.01%)	⬇️
output.go	82.95% <0.00%> (-6.62%)	⬇️
tx.go	76.08% <0.00%> (-1.50%)	⬇️
signaturehash.go	77.77% <0.00%> (-0.39%)	⬇️
input.go	82.25% <0.00%> (+4.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5e01aa...a279d95. Read the comment docs.

[jadwahab]

Thanks for raising this @caevv. From an initial glance, I don't see how your PR changes fix the issue raised. Can you give us some more information regarding the panic situation? Input values so that we can reproduce the panic error you mention above would be great.

[caevv]

Thanks for raising this @caevv. From an initial glance, I don't see how your PR changes fix the issue raised. Can you give us some more information regarding the panic situation? Input values so that we can reproduce the panic error you mention above would be great.

You are right, there is more to it than that. The size of that l seems to be bigger than what uint16 can hold. (65535 for this example)

I'm using this with tx id: cba1d7e24dd31cb9cb5d029b825b081e64c65d631ee8a71653a01210da7e1034

	for _, out := range tx.Outputs {
		_, err := out.LockingScript.ToASM()
		if err != nil {
			fmt.Println(err)
		}
	}

panics on LockingScript.ToASM:

panic: runtime error: slice bounds out of range [3:2]

goroutine 1 [running]:
github.com/libsv/go-bt/bscript.DecodeParts(0xc00039214e, 0x1001c, 0x11eb2, 0x0, 0x0, 0x0, 0x0, 0x0)
        .../github.com/libsv/go-bt/bscript/oppushdata.go:109 +0x1054
github.com/libsv/go-bt/bscript.(*Script).ToASM(0xc00000e240, 0x0, 0x0, 0x0, 0x0)
        .../github.com/libsv/go-bt/bscript/script.go:200 +0xaa

[ordishs]

Regarding this last comment about using binary.LittleEndian.Uint64(), this is incorrect. If you did make this change, we would read 8 bytes from the script rather than the 2 that we need for a uint16. Happy to explain further, if required. Regards Simon

…

On 6 May 2021, at 10:19, Mark Smith ***@***.***> wrote: @theflyingcodr commented on this pull request. In bscript/oppushdata.go <#22 (comment)>: > case OpPUSHDATA2: if len(b) < 3 { return r, errors.New("not enough data") } - l := binary.LittleEndian.Uint16(b[1:]) - if len(b) < int(3+l) { + + l := int(binary.LittleEndian.Uint16(b[1:])) Would probably be better to use binary.LittleEndian.Uint64(b[1:]) here and it should resolve this issue without needing to convert to int etc — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#22 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAROAU2XC6GAMNH4F6YO3LTMJGJ5ANCNFSM44FKVGMQ>.

[theflyingcodr]

Good point @ordishs; I think the other approach is probably best then.

Would you be able to add a unit test in with the failing script to show that the fix works and to ensure if this code is changed in future that the test catches it.