Skip to content

Conversation

LongShao007
Copy link
Contributor

when i debug it using VC, it will report "Expression: vector subscript out of range".

when i debug it using VC, it will report "Expression: vector subscript out of range".
@laanwj
Copy link
Member

laanwj commented May 28, 2014

This change is not correct.
VC is correct that the vector subscript is out of range, but in this case that is what we want, because we're trying to serialize the entire array.

@BitcoinPullTester
Copy link

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/34cb1fc5d6c7293b5c135b85c3dd3668e05ac2b8 for binaries and test log.

This could happen for one of several reasons:

  1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@haowang1013
Copy link

Shouldn't the correct fix be to still use &compr[compr.size()-1] while change CFlatData to use inclusive upper bound?

By definition compr[compr.size()] is a out of bound read even though it may not cause noticeable errors in certain implementations.

@laanwj
Copy link
Member

laanwj commented May 28, 2014

Inclusive upper bound is incredibly rare in computer science. Would be extremely confusing. It could be considered to pass a pointer and a size though.

Note that taking the address of compr[compr.size()] is not an out-of-bounds access in itself. The memory is never accessed.

@sipa
Copy link
Member

sipa commented May 28, 2014

Would using compr.end() work? It returns an iterator pointing to the same address as &compr[compr.size()].

@haowang1013
Copy link

Good point, I think compr.end() works the best in terms of satisfying the runtime as well as increasing readability.

I personally don't have preference between inclusive and exclusive upper bounds, none of them is more confusing than the other :)

@laanwj
Copy link
Member

laanwj commented May 28, 2014

The foremost problem with inclusive upper bounds is that they also require a record/type size. An exclusive upper bound is unambiguously the top of the address range encompassing the objects.

@laanwj
Copy link
Member

laanwj commented May 29, 2014

Using .end() makes sense here. But you'd still need something like &(*compr.begin()) and &(*compr.end()) to go from a vector random access iterator to a pointer. That may trigger yet another warning or may even fail in some cases if extra debug checking is compiled in.

Seemingly, getting the beginning and end pointer of a vector is not trivial to do in a way that is guaranteed to be portable and guaranteed to be working for empty vectors.

See also http://stackoverflow.com/a/1339767/216357 which introduces begin_ptr and end_ptr functions.

@LongShao007
Copy link
Contributor Author

ok, i will try!

@brandondahler
Copy link
Contributor

Why not use pointer arithmetic? This would prevent all warning messages, produce the same results as it currently is, and look purposeful (since it is).

s << CFlatData(&compr[0], (&compr[0]) + compr.size());

@laanwj
Copy link
Member

laanwj commented May 30, 2014

@brandondahler Did you check the link I gave? &compr[0] won't work portably for empty vectors (you're still referencing out of bounds in that case). That's why I referred to the stackoverflow topic which defines the following, which I think makes sense:

template <class T, class TAl>
inline T* begin_ptr(std::vector<T,TAl>& v)
{return  v.empty() ? NULL : &v[0];}

template <class T, class TAl>
inline const T* begin_ptr(const std::vector<T,TAl>& v)
{return  v.empty() ? NULL : &v[0];}

template <class T, class TAl>
inline T* end_ptr(std::vector<T,TAl>& v)
{return v.empty() ? NULL : (begin_ptr(v) + v.size());} 

template <class T, class TAl>
inline const T* end_ptr(const std::vector<T,TAl>& v)
{return v.empty() ? NULL : (begin_ptr(v) + v.size());}

@LongShao007 LongShao007 closed this Jun 3, 2014
@LongShao007 LongShao007 deleted the patch-3 branch June 3, 2014 14:29
laanwj added a commit to laanwj/bitcoin that referenced this pull request Jun 23, 2014
`&vch[vch.size()]` and even `&vch[0]` on vectors can cause assertion
errors with VC in debug mode. This is the problem mentioned in bitcoin#4239.
The deeper problem with this is that we rely on undefined behavior.

- Add `begin_ptr` and `end_ptr` functions that get the beginning and end
  pointer of vector in a reliable way that copes with empty vectors and
  doesn't reference outside the vector
(see https://stackoverflow.com/questions/1339470/how-to-get-the-address-of-the-stdvector-buffer-start-most-elegantly/1339767#1339767).
- Add a convenience constructor to CFlatData that wraps a vector.

I added `begin_ptr` and `end_ptr` as separate functions as I imagine
they will be useful in more places.
MathyV pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request Nov 3, 2014
`&vch[vch.size()]` and even `&vch[0]` on vectors can cause assertion
errors with VC in debug mode. This is the problem mentioned in bitcoin#4239.
The deeper problem with this is that we rely on undefined behavior.

- Add `begin_ptr` and `end_ptr` functions that get the beginning and end
  pointer of vector in a reliable way that copes with empty vectors and
  doesn't reference outside the vector
(see https://stackoverflow.com/questions/1339470/how-to-get-the-address-of-the-stdvector-buffer-start-most-elegantly/1339767#1339767).
- Add a convenience constructor to CFlatData that wraps a vector.

I added `begin_ptr` and `end_ptr` as separate functions as I imagine
they will be useful in more places.
reddink pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request May 27, 2020
`&vch[vch.size()]` and even `&vch[0]` on vectors can cause assertion
errors with VC in debug mode. This is the problem mentioned in bitcoin#4239.
The deeper problem with this is that we rely on undefined behavior.

- Add `begin_ptr` and `end_ptr` functions that get the beginning and end
  pointer of vector in a reliable way that copes with empty vectors and
  doesn't reference outside the vector
(see https://stackoverflow.com/questions/1339470/how-to-get-the-address-of-the-stdvector-buffer-start-most-elegantly/1339767#1339767).
- Add a convenience constructor to CFlatData that wraps a vector.

I added `begin_ptr` and `end_ptr` as separate functions as I imagine
they will be useful in more places.

(cherry picked from commit fa126ef)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants