Avoid undefined behavior in string/vector_move test#10365
Conversation
vec[1] is equivalent to vec[vec.size()] at that point and thus not a valid
element of the vector, making the use of operator[] undefined behavior here.
With some compiler flags (like those used in package builds on RHEL and
similar), the compiler (rightfully) aborts the program on this out of bounds
access:
68/178 Test #68: base-base_string/vector_move ............................................***Failed 0.01 sec
/usr/include/c++/14/bits/stl_vector.h:1130: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = icinga::String; _Alloc = std::allocator<icinga::String>; reference = icinga::String&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Running 1 test case...
unknown location(0): fatal error: in "base_string/vector_move": signal: SIGABRT (application abort requested)
/builds/packages/icinga2/packaging/fedora/41/BUILD/icinga2-2.14.5+467.g206d7cda1-build/icinga2-2.14.5+467.g206d7cda1/test/base-string.cpp(120): last checkpoint
*** 1 failure is detected in the test module "icinga2"
This commit fixes this by taking the indirection through .data() and using
plain pointer arithmetic instead.
|
Err! GitHub PR races 😅! Then please close #10363 when merging this PR! |
|
Well, I didn't see your PR until after I opened this one :D Any preference for either PR on your side? I'd say with your change, it's not that obvious why there's a second string inside the vector that isn't really used otherwise by the test. |
+1 for this PR! |
|
If the GitLab pipelines differ from GHA, don't the latter miss any compiler flags or similar? |
|
Yes, there's probably a difference in compile flags. Though this PR is about getting rid of the undefined behavior. |
|
Don't get me wrong, I'm also for getting rid off this UB, especially if it breaks our packaging. I'd just like to get notified about such ASAP – i.e. via GHA, not just when it's packaging time. For this I'd like us to first fix the GHA, so that it fails just like packaging does rn. |
|
Feel free to create a PR for that.
However, I don't see a reason why this should block this PR. |
|
I've created #10367 for the compiler flags. |
3 participants
vec[1]is equivalent tovec[vec.size()]at that point and thus not a valid element of the vector, making the use of operator[] undefined behavior here. With some compiler flags (like those used in package builds on RHEL and similar), the compiler (rightfully) aborts the program on this out of bounds access:This commit fixes this by taking the indirection through
.data()and using plain pointer arithmetic instead.This problem was recently introduced with PR #10353.
closes #10363
Tests
Current master branch
Failed snapshot pipeline for 206d7cd: https://git.icinga.com/packages/icinga2/-/pipelines/35807 (not publicly accessible)
This PR
Successful pipeline for 784867b: https://git.icinga.com/packages/icinga2/-/pipelines/35817 (not publicly accessible)