Fix out-of-bounds read in main (issue #1924) #1926

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2012_10_maindummybounds changing 1 files +1 −4

laanwj commented at 9:53 PM on October 12, 2012: member

Sizeof() returned the size of a pointer instead of the size of the buffer.

Fixes issue #1924.
luke-jr commented at 10:07 PM on October 12, 2012: member

Actually, I don't think it's this simple. With the new height-in-coinbase requirement, I expect either this can be left alone/null, or we need to put a height in there. I would have expected the latter, but it's obviously working right now for some (unknown to me) reason...
laanwj commented at 10:13 PM on October 12, 2012: member

It certainly cannot be left alone. Right now the dummy script is four bytes on 32 bit systems and eight bytes on 64 bit systems, and may contain undefined data (possible information leak or crash), which is obviously wrong.

If two bytes is not the right size, what is? (If eight bytes is working, safest may be to just fix it to eight zero bytes)
gavinandresen commented at 10:39 PM on October 12, 2012: contributor

RE: Luke-Jr's comment:

Should be scriptSig = CScript() << (pindexPrev->nHeight + 1) I suppose...

laanwj commented at 11:11 PM on October 12, 2012: member

This solution breaks the autotester:

pblock->vtx[0].vin[0].scriptSig = CScript((short)0);

With the following error:

Running 70 test cases...
unknown location(0): fatal error in "CreateNewBlock_validity": std::runtime_error: CreateNewBlock() : ConnectBlock failed
test/miner_tests.cpp(58): last checkpoint

So I went with:

pblock->vtx[0].vin[0].scriptSig = CScript() << OP_0 << OP_0;

This does pass all the tests.

Adding the depth should IMO be another pull, this one just fixes the immediate reported issue.

BitcoinPullTester commented at 3:23 PM on October 19, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a0ef4e1ee9807b9b553606778ba0884f3e602520 for binaries and test log.
Diapolo commented at 8:32 PM on October 19, 2012: none

It's scary that only @BitcoinPullTester is active in the project, what's up? Did I miss something?
laanwj commented at 6:20 AM on October 20, 2012: member

Everything (at least core changes) are in hibernation waiting for ultraprune (#1677) to be merged :smile_cat:
Diapolo commented at 8:34 AM on October 20, 2012: none

That patch scares me even more ^^...
laanwj commented at 9:01 AM on October 20, 2012: member

It is scary, but it is a necessity. It helps against some long-running performance issues and will allow for new features. We can make it somewhat less scary by testing and reviewing it extensively, that's why other things are on hold.

Fix out-of-bounds read noticed by Ricardo Correia

Sizeof() returned the size of a pointer instead of the size of the buffer.
Fixes issue #1924.

4fbad9124e

laanwj commented at 6:27 AM on October 24, 2012: member

Rebased, can be merged again
sipa commented at 9:17 AM on October 24, 2012: member

ACK
gavinandresen commented at 2:18 PM on October 24, 2012: contributor

ACK
laanwj referenced this in commit 1f7c5c5a3e on Oct 25, 2012
laanwj merged this on Oct 25, 2012
laanwj closed this on Oct 25, 2012
laudney referenced this in commit 5d3028dd11 on Mar 19, 2014
laanwj deleted the branch on Apr 9, 2014
KolbyML referenced this in commit ab460a523f on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1924 Read out of bounds in main.cpp:3462