12 ms·

asan/ubsan are great for finding language-level undefined behavior. But apart from some functions like memcpy that asan is hooking, they cannot find library-lev

by ynik 4y ago

asan/ubsan are great for finding language-level undefined behavior. But apart from some functions like memcpy that asan is hooking, they cannot find library-level undefined behavior such as the pop_back() on the empty vector. (That's an inline function inside a template, there's no way asan could hook this!) I recommend using `-D_GLIBCXX_ASSERTIONS` in addition to `-fsanitize=undefined,address`. This will enable some cheap assertions in libstdc++ that catch stuff like the invalid pop_back() call. In fact it might not be a bad idea to keep these assertions enabled in release builds! (There is also `-D_GLIBCXX_DEBUG` which enables even more assertions, but some of those are expensive)

gpderetta 4y ago
Also, it did still catch a problem in the second scenario, it just didn't conveniently point to the right line. +1 on _GLIBXCXX_{ASSERTION,DEBUG}.
- gfd 4y ago
  According to https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_u... there's a D_GLIBCXX_DEBUG_BACKTRACE that will show a backtrace for the line number too
roel_v 4y ago
Meta: when you ask chatgpt what happens when you do pop_back() on an empty vector, it confidently claims std::out_of_range will be thrown. It will even 'cite' the standard with a plausible explanation of what will happen (essentially, it says that a prereq is that empty() == false, and that it depends on what erase() throws). https://cplusplus.com/reference/vector/vector/pop_back/ https://cplusplus.com/reference/vector/vector/pop_back/ and other sources I can find say undefined behavior. I don't have a copy of the standard at hand, can anyone quote the relevant section? Is this something that in practice is implemented in different (exception-throwing) ways?
- gpderetta 4y ago
  > Is this something that in practice is implemented in different (exception-throwing) ways? I don't think so. Even when enabling debugging mode, a precondition failure would cause an abort, not an exception to be thrown.
- saurik 4y ago
  God help us if people start trying to use an overconfident chatbot to replace documentation :(.
  - roel_v 4y ago
    Maybe. To be fair, I had the same opinion yesterday. This morning I decided to take a real look at how I could use chatgpt to augment my programming, and once I started with it with an open mind, I now think it's going to take away much of the drudgery I have come to loathe so much about programming. I used to like delving into libraries and frameworks and learning all their details - I thought that was what 'learning about programming' was. Now, 20 years later, I hate those aspects with a passion - having to look up the idiosyncrasies of a function call again, because there are dozens of ways of using it but all slightly different, or piecing together another dull specific use case. This morning I had chatgpt generate 4 fully fledged functions for me, each doing a highly specific task that would have taken maybe 20 or 30 minutes each to piece together the specific API's, and one of them using a Python package I had never used before, didn't know of and would have had to read the getting started tutorial before I could have used it on my own. Then all I had to do was write the glue to call those functions in the right order and with the right arguments and boom that was it. Plus the code was more elaborate and coherent than it would have been had I have to do it manually. One example is not a guarantee, but the experience made my day, and probably my week. Which is why the pop_back() thing made me worry :)
    - Someone 4y ago
      > This morning I had chatgpt generate 4 fully fledged functions for me, each doing a highly specific task that would have taken maybe 20 or 30 minutes each to piece together the specific API's, and one of them using a Python package I had never used before, didn't know of and would have had to read the getting started tutorial before I could have used it on my own. Then all I had to do was write the glue to call those functions in the right order and with the right arguments and boom that was it. “Boom that was it” _if_ you blindly trust ChatGPT to generate correct code. If you don’t (and IMO you shouldn’t), “it worked for me today” isn’t sufficient to decide whether you can trust it. You should go to the documentation of that package, judge whether ChatGPT used it correctly (and yes, that involves looking up the idiosyncrasies of function calls), judge whether you trust that package, evaluate its license, etc.
planede 4y ago
AFAIK `_GLIBCXX_DEBUG` alters ABI to the library, it should be only used with a libstdc++ and all other components compiled with the same flag. Debug iterators, and all that jazz. MSVC users are probably not surprised. `_GLIBCXX_ASSERTIONS` does not alter ABI.
- wyldfire 4y ago
  From https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros... > Undefined by default. When defined, compiles user code using the debug mode. "user code" here seems to indicate it's safe for libstdc++ users to define without worrying about ABI changes. If there were compatibility concerns, I'd expect that to be described in the documentation link above. A strong distinction between _GLIBCXX_DEBUG and _GLIBCXX_ASSERTIONS is that the latter tries not to change the big-O complexity of operations when defined.
  - gpderetta 4y ago
    very technically defining _GLIBCXX_DEBUG in different ways across translation units violates the ODR, so it is UB. In practice this is expected to work with GCC[1] and the and the macro is ABI safe. [1] if you are unlucky the function might not be inlined and an instantiantion form another translation unit that was compiled with the assert disabled might be picked up and the assert not fire.
    - planede 4y ago
      Well, if we are nitpicking here, defining _GLIBCXX_ASSERTIONS in different ways across translation units also potentially violates ODR, as some inline functions or function templates get different definitions in different translation units. This still can have undesired effects. An inline function can end up not being inlined in two such translation units, and depending on how the linker resolves the weak symbols you might get diagnostics in a TU where you didn't enable it or the other way around. But this manifestation is arguably much more benign than the ones you get for changing the layout of some classes. edit: In general ABI stability is not ODR stability. You can break ODR and retain ABI stability. You can't even add a non-virtual member function to a class without breaking ODR, but in most if not all ABIs this is fine. The standard of course does not acknowledge the existence of "benign" ODR breaks, or the existence of an ABI.
Ygg2 4y ago
As someone coming from Java, why is pop_back undefined behavior? An error or noop is something I expect, but undefined behavior?
- dezgeg 4y ago
  It presumably allows the implementation to be a tiny bit faster, by avoiding a compare and branch for the empty case.
- st_goliath 4y ago
  I suppose it causes an integer underflow and tryies to in-place destruct in an empty buffer? The std::vector class is somewhat inconsistent about safety and maps a lot of it's operations directly to unsafe, raw array access. The C++ STL contains a lot of historical baggage and design inconsistencies. For instance, while std::vector has an `at(index)` method, that does proper bounds checking and throws an exception if the index is out of range, it also has an overloaded `operator []` that mimics array syntax that will happily write out of bounds, trashing the heap.
  - gpderetta 4y ago
    STL was designed to have no overhead over the corresponding C code. If push_back (and all other functions) had an extra check even when it was guaranteed non-empty by construction, it would have been a non-starter for many users. Consider that it took many years for compilers to reach the near zero overhead for STL as it is, removing precondition checks would have been beyond the ability of many compilers 20 years ago, and it still challenging today. Of course the actual overhead went down since and today we expect code to be more robust by default, but the original design principles were different. If you want safety by default, check your compiler documentation and compile with lightweight assertions enabled even in release builds.
    - simiones 4y ago
      This makes sense for std::vector::operator[], but not for pop_back and push_back - as those don't have any equivalent for a C array. Instead, it's just another example of C++ optimizing for speed as opposed to correctness.
simiones 4y ago
So, if following the recommended C++ practices of mostly using std containers, ASan and UBSan are mostly useless?
- synergy20 4y ago
  containers are not thread safe though, so the thread safe sanitizer will be useful there. ASAN and UBSAN are supposed to be handled by containers internally with RAII if they're used correctly, I still turn on both ASAN and UBSAN to catch bugs, it does not hurt.
  - simiones 4y ago
    As shown in the article, the following code invokes UB in user code, and is not directly detected by ASAN or UBSAN: std::vector<int> c; c.pop_back(); //pop_back() on empty vector is UB, per the library documentation int j = c[0]; //also UB, not sure if would be detected This code is fully RAII compliant, but wrong.
    - menaerus 4y ago
      It only takes a tiny effort to prove your ignorance with the exact example you gave. https://gcc.godbolt.org/z/x4sbTWxrY https://gcc.godbolt.org/z/x4sbTWxrY ASM generation compiler returned: 0 Execution build compiler returned: 0 Program returned: 0 /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_vector.h:1320:2: runtime error: applying non-zero offset 18446744073709551612 to null pointer /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_vector.h:1124:34: runtime error: reference binding to null pointer of type 'value_type' /app/example.cpp:7:14: runtime error: load of null pointer of type 'value_type'
jcelerier 4y ago
> asan/ubsan are great for finding language-level undefined behavior. But apart from some functions like memcpy that asan is hooking, they cannot find library-level undefined behavior such as the pop_back() on the empty vector. (That's an inline function inside a template, there's no way asan could hook this!) no, libstdc++ is at least partly instrumented with explicit asan support : https://gcc.gnu.org/legacy-ml/libstdc++/2017-07/msg00014.html https://gcc.gnu.org/legacy-ml/libstdc++/2017-07/msg00014.htm... so fsanitize should be able to catch it (and after checking, definitely does: https://gcc.godbolt.org/z/rjadKvPTs https://gcc.godbolt.org/z/rjadKvPTs)
- ynik 4y ago
  That's a special case due to an empty vector using nullptr, and thus causing language-level UB when doing pointer arithmetic with a nullptr. If you add an element to the vector and then remove it twice, asan will no longer find any problem: https://gcc.godbolt.org/z/MbPhWv8vn https://gcc.godbolt.org/z/MbPhWv8vn
  - jcelerier 4y ago
    ahhh just double checked and it is hidden behind a macro for libstdc++... see: https://gcc.godbolt.org/z/e7Wj163PM https://gcc.godbolt.org/z/e7Wj163PM
    - ynik 4y ago
      Interesting, I wasn't aware of that one. I think `-D_GLIBCXX_ASSERTIONS` is more appropriate for catching this bug, but `-D_GLIBCXX_SANITIZE_VECTOR` seems useful for cases like this: https://gcc.godbolt.org/z/EjrdoTYPs https://gcc.godbolt.org/z/EjrdoTYPs This is really the main problem with "safe modern C++": to get actual safety you need dozens of weird implementation-specific opt-ins and there's no good way to learn that these even exist.

by ynik 4y ago

gpderetta 4y ago

Also, it did still catch a problem in the second scenario, it just didn't conveniently point to the right line. +1 on _GLIBXCXX_{ASSERTION,DEBUG}.

gfd 4y ago
According to https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_u... there's a D_GLIBCXX_DEBUG_BACKTRACE that will show a backtrace for the line number too

roel_v 4y ago

Meta: when you ask chatgpt what happens when you do pop_back() on an empty vector, it confidently claims std::out_of_range will be thrown. It will even 'cite' the standard with a plausible explanation of what will happen (essentially, it says that a prereq is that empty() == false, and that it depends on what erase() throws). https://cplusplus.com/reference/vector/vector/pop_back/ https://cplusplus.com/reference/vector/vector/pop_back/ and other sources I can find say undefined behavior. I don't have a copy of the standard at hand, can anyone quote the relevant section? Is this something that in practice is implemented in different (exception-throwing) ways?

gpderetta 4y ago
> Is this something that in practice is implemented in different (exception-throwing) ways? I don't think so. Even when enabling debugging mode, a precondition failure would cause an abort, not an exception to be thrown.
saurik 4y ago
God help us if people start trying to use an overconfident chatbot to replace documentation :(.
- roel_v 4y ago
  Maybe. To be fair, I had the same opinion yesterday. This morning I decided to take a real look at how I could use chatgpt to augment my programming, and once I started with it with an open mind, I now think it's going to take away much of the drudgery I have come to loathe so much about programming. I used to like delving into libraries and frameworks and learning all their details - I thought that was what 'learning about programming' was. Now, 20 years later, I hate those aspects with a passion - having to look up the idiosyncrasies of a function call again, because there are dozens of ways of using it but all slightly different, or piecing together another dull specific use case. This morning I had chatgpt generate 4 fully fledged functions for me, each doing a highly specific task that would have taken maybe 20 or 30 minutes each to piece together the specific API's, and one of them using a Python package I had never used before, didn't know of and would have had to read the getting started tutorial before I could have used it on my own. Then all I had to do was write the glue to call those functions in the right order and with the right arguments and boom that was it. Plus the code was more elaborate and coherent than it would have been had I have to do it manually. One example is not a guarantee, but the experience made my day, and probably my week. Which is why the pop_back() thing made me worry :)
  - Someone 4y ago
    > This morning I had chatgpt generate 4 fully fledged functions for me, each doing a highly specific task that would have taken maybe 20 or 30 minutes each to piece together the specific API's, and one of them using a Python package I had never used before, didn't know of and would have had to read the getting started tutorial before I could have used it on my own. Then all I had to do was write the glue to call those functions in the right order and with the right arguments and boom that was it. “Boom that was it” _if_ you blindly trust ChatGPT to generate correct code. If you don’t (and IMO you shouldn’t), “it worked for me today” isn’t sufficient to decide whether you can trust it. You should go to the documentation of that package, judge whether ChatGPT used it correctly (and yes, that involves looking up the idiosyncrasies of function calls), judge whether you trust that package, evaluate its license, etc.

planede 4y ago

AFAIK `_GLIBCXX_DEBUG` alters ABI to the library, it should be only used with a libstdc++ and all other components compiled with the same flag. Debug iterators, and all that jazz. MSVC users are probably not surprised. `_GLIBCXX_ASSERTIONS` does not alter ABI.

wyldfire 4y ago
From https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros... > Undefined by default. When defined, compiles user code using the debug mode. "user code" here seems to indicate it's safe for libstdc++ users to define without worrying about ABI changes. If there were compatibility concerns, I'd expect that to be described in the documentation link above. A strong distinction between _GLIBCXX_DEBUG and _GLIBCXX_ASSERTIONS is that the latter tries not to change the big-O complexity of operations when defined.
- gpderetta 4y ago
  very technically defining _GLIBCXX_DEBUG in different ways across translation units violates the ODR, so it is UB. In practice this is expected to work with GCC[1] and the and the macro is ABI safe. [1] if you are unlucky the function might not be inlined and an instantiantion form another translation unit that was compiled with the assert disabled might be picked up and the assert not fire.
  - planede 4y ago
    Well, if we are nitpicking here, defining _GLIBCXX_ASSERTIONS in different ways across translation units also potentially violates ODR, as some inline functions or function templates get different definitions in different translation units. This still can have undesired effects. An inline function can end up not being inlined in two such translation units, and depending on how the linker resolves the weak symbols you might get diagnostics in a TU where you didn't enable it or the other way around. But this manifestation is arguably much more benign than the ones you get for changing the layout of some classes. edit: In general ABI stability is not ODR stability. You can break ODR and retain ABI stability. You can't even add a non-virtual member function to a class without breaking ODR, but in most if not all ABIs this is fine. The standard of course does not acknowledge the existence of "benign" ODR breaks, or the existence of an ABI.

Ygg2 4y ago

As someone coming from Java, why is pop_back undefined behavior? An error or noop is something I expect, but undefined behavior?

dezgeg 4y ago
It presumably allows the implementation to be a tiny bit faster, by avoiding a compare and branch for the empty case.
st_goliath 4y ago
I suppose it causes an integer underflow and tryies to in-place destruct in an empty buffer? The std::vector class is somewhat inconsistent about safety and maps a lot of it's operations directly to unsafe, raw array access. The C++ STL contains a lot of historical baggage and design inconsistencies. For instance, while std::vector has an `at(index)` method, that does proper bounds checking and throws an exception if the index is out of range, it also has an overloaded `operator []` that mimics array syntax that will happily write out of bounds, trashing the heap.
- gpderetta 4y ago
  STL was designed to have no overhead over the corresponding C code. If push_back (and all other functions) had an extra check even when it was guaranteed non-empty by construction, it would have been a non-starter for many users. Consider that it took many years for compilers to reach the near zero overhead for STL as it is, removing precondition checks would have been beyond the ability of many compilers 20 years ago, and it still challenging today. Of course the actual overhead went down since and today we expect code to be more robust by default, but the original design principles were different. If you want safety by default, check your compiler documentation and compile with lightweight assertions enabled even in release builds.
  - simiones 4y ago
    This makes sense for std::vector::operator[], but not for pop_back and push_back - as those don't have any equivalent for a C array. Instead, it's just another example of C++ optimizing for speed as opposed to correctness.

simiones 4y ago

So, if following the recommended C++ practices of mostly using std containers, ASan and UBSan are mostly useless?

synergy20 4y ago
containers are not thread safe though, so the thread safe sanitizer will be useful there. ASAN and UBSAN are supposed to be handled by containers internally with RAII if they're used correctly, I still turn on both ASAN and UBSAN to catch bugs, it does not hurt.
- simiones 4y ago
  As shown in the article, the following code invokes UB in user code, and is not directly detected by ASAN or UBSAN: std::vector<int> c; c.pop_back(); //pop_back() on empty vector is UB, per the library documentation int j = c[0]; //also UB, not sure if would be detected This code is fully RAII compliant, but wrong.
  - menaerus 4y ago
    It only takes a tiny effort to prove your ignorance with the exact example you gave. https://gcc.godbolt.org/z/x4sbTWxrY https://gcc.godbolt.org/z/x4sbTWxrY ASM generation compiler returned: 0 Execution build compiler returned: 0 Program returned: 0 /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_vector.h:1320:2: runtime error: applying non-zero offset 18446744073709551612 to null pointer /opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/bits/stl_vector.h:1124:34: runtime error: reference binding to null pointer of type 'value_type' /app/example.cpp:7:14: runtime error: load of null pointer of type 'value_type'

jcelerier 4y ago

> asan/ubsan are great for finding language-level undefined behavior. But apart from some functions like memcpy that asan is hooking, they cannot find library-level undefined behavior such as the pop_back() on the empty vector. (That's an inline function inside a template, there's no way asan could hook this!) no, libstdc++ is at least partly instrumented with explicit asan support : https://gcc.gnu.org/legacy-ml/libstdc++/2017-07/msg00014.html https://gcc.gnu.org/legacy-ml/libstdc++/2017-07/msg00014.htm... so fsanitize should be able to catch it (and after checking, definitely does: https://gcc.godbolt.org/z/rjadKvPTs https://gcc.godbolt.org/z/rjadKvPTs)

ynik 4y ago
That's a special case due to an empty vector using nullptr, and thus causing language-level UB when doing pointer arithmetic with a nullptr. If you add an element to the vector and then remove it twice, asan will no longer find any problem: https://gcc.godbolt.org/z/MbPhWv8vn https://gcc.godbolt.org/z/MbPhWv8vn
- jcelerier 4y ago
  ahhh just double checked and it is hidden behind a macro for libstdc++... see: https://gcc.godbolt.org/z/e7Wj163PM https://gcc.godbolt.org/z/e7Wj163PM
  - ynik 4y ago
    Interesting, I wasn't aware of that one. I think `-D_GLIBCXX_ASSERTIONS` is more appropriate for catching this bug, but `-D_GLIBCXX_SANITIZE_VECTOR` seems useful for cases like this: https://gcc.godbolt.org/z/EjrdoTYPs https://gcc.godbolt.org/z/EjrdoTYPs This is really the main problem with "safe modern C++": to get actual safety you need dozens of weird implementation-specific opt-ins and there's no good way to learn that these even exist.