Refactor: simpler read #11221

pull gnuser wants to merge 1 commits into bitcoin:master from gnuser:refactor-streams changing 1 files +5 −7

gnuser commented at 5:46 AM on September 3, 2017: contributor

No description provided.
Refactor: make the read function simpler 9db9d6215f
in src/streams.h:351 in 9db9d6215f
```
 344 | @@ -345,18 +345,16 @@ class CDataStream
 345 |  
 346 |          // Read from the beginning of the buffer
 347 |          unsigned int nReadPosNext = nReadPos + nSize;
 348 | -        if (nReadPosNext >= vch.size())
 349 | +        if (nReadPosNext > vch.size()) {
 350 | +            throw std::ios_base::failure("CDataStream::read(): end of data");
 351 | +        }
 352 | +        memcpy(pch, &vch[nReadPos], nSize);        
```
sipa commented at 6:44 AM on September 3, 2017:

This is not correct. When vch is empty, and nReadPos and nSize are 0, this will invoke operator[] on vch, which is not valid.

meshcollider commented at 7:34 AM on September 3, 2017:

Note that that's a bug with the original code though, not with the refactor

laanwj commented at 5:42 PM on September 6, 2017:

probably better to use vch.data() + nNreadPos? (but yes, this is not a bug introduced in this PR)

promag commented at 4:22 PM on October 8, 2017:

@sipa above there is an early return if nSize == 0 so that doesn't happen. Out of bounds access is prevented with the std::ios_base::failure exception.

sipa commented at 11:27 PM on October 12, 2017:

Agree, there is no bug here. It would still be clearer using vch.data() + nReadPos though.
dcousens approved
laanwj added the label Refactoring on Sep 5, 2017
promag commented at 4:24 PM on October 8, 2017: member

utACK 9db9d62.

Nit, commit message could mention CDataStream::read instead read only.

in src/streams.h:352 in 9db9d6215f

 344 | @@ -345,18 +345,16 @@ class CDataStream
 345 |  
 346 |          // Read from the beginning of the buffer
 347 |          unsigned int nReadPosNext = nReadPos + nSize;
 348 | -        if (nReadPosNext >= vch.size())
 349 | +        if (nReadPosNext > vch.size()) {
 350 | +            throw std::ios_base::failure("CDataStream::read(): end of data");
 351 | +        }
 352 | +        memcpy(pch, &vch[nReadPos], nSize);        
 353 | +        if (nReadPosNext == vch.size())

promag commented at 4:24 PM on October 8, 2017:

Nit, { on this line.

laanwj merged this on Nov 9, 2017
laanwj closed this on Nov 9, 2017
laanwj referenced this in commit 0dec4cc300 on Nov 9, 2017
jnewbery cross-referenced this on Nov 15, 2017 from issue [trivial] remove trailing whitespace in streams.h by jnewbery
PastaPastaPasta referenced this in commit 0f978bab3b on Jan 31, 2020
PastaPastaPasta referenced this in commit fe38d4f5ee on Jan 31, 2020
PastaPastaPasta referenced this in commit e2137607bf on Feb 4, 2020
PastaPastaPasta referenced this in commit 2a018f8807 on Feb 9, 2020
furszy cross-referenced this on Sep 27, 2020 from issue BugFix: tx sync parsing problem fixed + extra refactoring. by furszy
random-zebra referenced this in commit 8e7fa721af on Sep 27, 2020
ckti referenced this in commit 31e45c0ace on Mar 28, 2021
gades referenced this in commit a3194e3717 on Jun 30, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11697 [trivial] remove trailing whitespace in streams.h