15 ms·

Can sanitizers find the two bugs I wrote in C++?

138 points by signa11 4y ago

deleted 4y ago
[deleted]
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.
zabzonk 4y ago
well, one of the problems could have been detected with "grep atoi source_code_here". any code that uses atoi is almost certainly bad (no error detection), and C++ provides several alternatives.
- jeffbee 4y ago
  Right? Why do people love these C library functions? I think the biggest problem in the C++ community is that some people know a little C. The memcpy of the vector’s data sounds like it also might be non idiomatic, but I don’t see the source code so I am not sure.
  - gpderetta 4y ago
    Ideally the compiler would suggest to replace atoi with the C++ equivalent. But not even clang-tidy does with the set of warnings I have enabled.
    - pclmulqdq 4y ago
      That's not the philosophy of the C++ compiler writers. There are linters that suggest it, but compilers don't do this sort of warning.
  - zabzonk 4y ago
    i don't even understand why atoi gets used in pure C code - there are much better standard C alternatives.
  - eklitzke 4y ago
    In this case the code was using the .data() on a std::vector<char>, so it wouldn't have even been possible to use something like std::stoi (or something nicer like absl::SimpleAtoi) since the C++ routines need a std::string (or absl will accept std::string_view), not a C style string. Which in itself shows you that the code is problematic, because it's been written in a way where you can't even use idiomatic C++ library functions to do basic tasks. For the memcpy() thing the idiomatic way would have been to use std::copy() with the vectors begin/end iterators, and std::copy will do the right thing in this situation with a zero sized vector. The compiler will generate the same code with std::copy() as memcpy() anyway if the vector contains a POD type (as it did in this example), so there's not even a performance argument to be made for memcpy().
tubs 4y ago
LD_PRELOAD is probably want you want.
omnicognate 4y ago
Enabling libstdc++ debug mode (-D_GLIBCXX_DEBUG) would probably have located the second one, assuming gcc. It adds bounds and other checks to containers like std::vector. [1] Edit: The article mentions clang, so probably [2] is the relevant link. [1] https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_u... [2] https://libcxx.llvm.org/DesignDocs/DebugMode.html https://libcxx.llvm.org/DesignDocs/DebugMode.html
- account42 4y ago
  You can use libstdc++ with Clang (-stdlib=libstdc++) and libc++ with GCC (-nostdlib along with adding the libraries and include paths manually). In fact, libstdc++ tends to be the default for Clang installs on Linux for ABI-compatibility with GCC-compiled C++ libraries unless you pass -stdlib=libc++.
meindnoch 4y ago
Don't use atoi. Use std::from_chars.
- account42 4y ago
  This makes sense for integers but beware floating point from_chars - libc++ still doesn't implement it and libstdc++ implements it by wrapping locale-dependent libc functions which involves temporarily changing the thread locale and possibly memory allocation to make the passed string 0-terminated. IMO libstdc++'s checkbox "solution" is worse than not implementing it at all - user's are better off using Lemire's API-compatible fast_float implementation [0]. Of course it's possible that the author wanted to support compilers released before 2018 when that code was written in which case not even integer from_chars would be available. [0] https://github.com/fastfloat/fast_float https://github.com/fastfloat/fast_float
  - meindnoch 4y ago
    Hm. Good to know! What about strtof?
    - zabzonk 4y ago
      the strtoX functions have been around for many, many years, and should be completely reliable. but i would use strtod. in fact i never use floats in my code unless i am dealing with very large numbers of floating-point values.
- simiones 4y ago
  Is there an alternative if not using C++17 or newer?
  - zabzonk 4y ago
    several (or should that be many?) - for example stoi, sscanf, operator >>(istream &, int &) - there are others.
- vitus 4y ago
viraptor 4y ago
I wonder if PVS would flag those issues. (https://pvs-studio.com/en/ https://pvs-studio.com/en/) I expect it to flag the atoi, but not sure about the pop.
jussij 4y ago
This is the beauty of modern C++. The code has become so hard to read (without years of prior knowledge) that as a human, it's rather hard to find any possible errors. The code can take on a Schrodinger state where it appears both right and wrong.
- simplotek 4y ago
  The code is not hard to read. People write code that is hard to read. English is a great language, but some people write stuff down in good plain English that is hard to parse. The same goes for any programming language. If you want to write code that's hard to read, that's what you will get.
  - bArray 4y ago
    I don't not think that English isn't difficult to parse. I agree completely that it's all about the code you write. You can write nice C++ that is easy to understand, easy to debug, and that you can be quite certain is bug free. Something like Rust appears to eliminate one class of bugs, but then people still write bugs. Just look at the Rust language issue tracker on GitHub [1]. Fundamentally, you cannot get around the need for good code design. Any useful programming language will always have the ability to mishandle data. [1] https://github.com/rust-lang/rust/issues https://github.com/rust-lang/rust/issues
    - galangalalgol 4y ago
      Rust doesn't create any new classes of bugs, and it eliminates some old ones. Isn't that all positive? Rust also turns a lot more bugs from UB and/or exploitable, into a panic. That is still potentially a ddos, but it is better than code injection. And therebare tools like prusti to find panics, and you can use tricks with linking a nostd library to see if you got them all. Probably better ways than that now. We can obviously write bugs in any language, but it is just as obvious that some languages make finding some bugs easier. Rust makes some bugs easy to find, the compuler points them out, and unless it makes other bugs harder to find it is all upside (from the correctness standpoint, not all standpoints)
pornel 4y ago
Is there a C++ proposal to add mandatory bounds checks to methods like pop_back? If modern C++ is serious about getting memory safety, UB like this should be changed to have a run-time check and abort. This is even backwards compatible, cheap or zero cost — if it's used in a loop that checks for non-empty length, the condition could be propagated and the redundant check optimized out. I assume it won't get fixed, because from what I've seen so far if there is even slightest whiff of a performance overhead, modern C++ still chooses unsafety. If there's any inertia or backwards compatibility worry, even if it's a spacebar-heater, modern C++ still chooses to keep the old unsafety. Safety is still not a priority.
- aw1621107 4y ago
  At least based on this comment from the maintainer of the C++ NanoRanges library [0], it seems contracts (at one point) may be the intended method to bounds check: > I actually proposed at() for std::span as part of P1024r0. It was soundly defeated in LEWG, 0|0|4|4|5. > I wasn’t there for the meeting, but from what I understand the feeling was that contracts (which at that time seemed highly likely for ‘20) were a better way to go for bounds checking, and that we should not be throwing exceptions for programmer errors like out-of-bounds access. No idea whether that is still the case though. [0]: https://old.reddit.com/r/cpp/comments/djqdu2/why_is_stdspan_is_missing_at/f48hhb5/ https://old.reddit.com/r/cpp/comments/djqdu2/why_is_stdspan_...
  - bluGill 4y ago
    I'm not sure what the current thought is, but I'm hopeful contracts makes it into C++, and over a the next years (and iterations of the language) we find ways to make C++ better by proving the whole program correct. This will need a lot of tool support, and changes in the way people write code. However mathematical proofs of the program are the only thing I have left to make my code more reliable: humans are not perfect and unit tests have limits (proofs have different limits).
    - WanderPanda 4y ago
      I don't entirely get the concept between mathematical proofs of programs. Is the basic idea that it is straight forward for humands to write down a correct NP hard specification and then let some algorithm proof the correctness of a more efficient algorithm for the same problem?
inetknght 4y ago
This post supports my personal experience. ASAN is a flippin' godsend. I have never encountered a false report from ASAN. I have seen it have false negatives but those are usually intermittent with true positives when the tests are run again and usually related to very small buffer overflows (eg, a "few bytes" past the end of a buffer). Not perfect but no false positives means it's 100% worthwhile to have enabled. But UBSAN is... not as helpful. It sometimes catches things. It often misses some undefined behavior that can be easily identified elsewhere. But the real kicker is that (almost?) all of the sanitizers are mutually exclusive. That really sucks if you're trying to debug something very complex. It also has lead me to build and drive the complexity into smaller and smaller chunks. Arguably that's a good thing! I have, for several years now, always turned Address Sanitizer on by default when -DCMAKE_BUILD_TYPE=Debug or off when it's not. I've added an option that forces the option with -DASAN_BUILD=bool. I wish Kitware would simplify my CMakeLists files by making this default behavior. And I think it would really help a lot of open source projects that aren't configured for ASAN too
- cyber_kinetist 4y ago
  I think there’s a place for Address Sanitizer to be enabled on RelWithDbgInfo build type (usually used for development and not production). Debug mode in C++ is already too slow, but adding a sanitizer makes it almost unusable for performance-sensitive code.
  - kllrnohj 4y ago
    The sanitizers are not incompatible with optimizations. asan with -O2 is a perfectly valid & useful combination. It's much faster than valgrind or anything like that, very usable levels of performance. And on an increasingly common number of CPUs there's hardware acceleration for ASAN as well ( https://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html https://clang.llvm.org/docs/HardwareAssistedAddressSanitizer... & https://source.android.com/docs/security/test/hwasan https://source.android.com/docs/security/test/hwasan )
- mannykannot 4y ago
  With regard to UBSAN, are the misses that you see examples of specific checkers failing to find the specific sort of UB that they are intended to detect? If so, do the majority of misses seem to come from a small subset of the checkers?
wyldfire 4y ago
Another option for dynamic bounds checking on heap allocations is Electric Fence or scudo. It probably wouldn't find that issue with pop_front() though.
axilmar 4y ago
The problem of hidden types surfaces again. Obviously passing null to a function that does not expect null is a well known problem, tackled by many programming languages but not by C and C++. This is a purely typing issue, because null is a different thing than a valid memory address. The other issue about vector.pop_back() is also a hidden type issue...an empty vector is not the same as a non-empty vector, and therefore pop_back() applies only to non-empty vector. What would a possible solution be in the context of C++? Well, each function should only be applicable when the parameters are in the states the function expects. So parameters (including this/self) shall have states. For example: void pop_back() this : not_empty { ... } But this is not enough. The language should allow the user to specify change of states at certain statements/expressions. For example: void clear() { ... this := empty; } And then the compiler can check the AST if the used vector has the 'empty' state on pop back or not. Doing the following would be illegal: vector<int> data; data.clear(); data.pop_back(); And the following would be illegal because one execution path would lead to a state violation: vector<int> data; if (some_external_condition) { data.clear(); } data.pop_back(); And the following would be illegal too, because the result of clear_random_vector could be possible in empty state. vector<int>& clear_random_vector(vector<int>& a, vector<int>& b) { if (rand() < RAND_MAX / 2) { //c++ has better random number interface a.clear(); return a; } b.clear(); return b; } int main() { vector<int> a{1, 2, 3}, b{4, 5, 6}; vector<int>&v = clear_random_vector(a, b); v.pop_back(); } And in case the function clear_random_vector is in another translation unit, then: a) if headers are used, the programmer shall provide metadata about the state of the result in the header. Example: vector<int>& clear_random_vector(vector<int>& a, vector<int>& b) := empty; b) if modules are used, the compiler itself should output the metadata. Actually, this would also solve the null pointer problem, because a pointer would have the states empty, not_empty by default, and then functions could be like this: memcpy(void* dst : not_empty, const void* src : not_empty, size_t count); And then this would be caught at compile time: std::vector<char> dst, src; memcpy(dst.data(), src.data(), 10); States could also solve the problem of array indexing. For example, an index can have two states, 'in_range' and 'out_of_range'. Using an integer as an array index would implicitly add the two states in the index variable; comparing the integer with the array range would automatically add 'in_range' or 'out_of_range' to the integer. Example of valid usage: vector<int> data{1, 2, 3}; int i = 0; //our index //'i' gets automatically 'in_range' state. if (i < data.size()) { std::cout << data[i] << std::endl; } The following would be invalid, because straightforward array indexing would requre a state named 'in-range': vector<int> data{1, 2, 3}; int i = 0; //our index std::cout << data[i] << std::endl; How would that work at language level? well, here is an idea: a) any operation can affect the compile-time state of objects, b) states themselves would be templates, bound to certain ids at compile time. Here is a possible implementation: template <class T> class vector { //returns a 'size_t' variable tagged with state 'size_of_this' and parameter 'this'; in this case, this does not refer to the runtime address of the object, but to the identifier this object is bound to. size_t size() const : size_of_this<this> { ... } } //index is bound to state bound_index<T>, where T refers to the variable of the vector at compile time. template <T> operator < (int& index, size_t size: size_of_this<T>) { index := size_of_this<T>; } template <class T> class vector { //the vector operator [i] requires size_of_this<T> const T& operator [](int index : size_of_this<this>) const { ... } //adding an element to the vector breaks size_of_this. void push_back(const T& elem) { this := !size_of_this<this>; } } So, each time push_back is called on a vector, the guarantee about indexes breaks, and the index that previously holds the guarrantee can no longer be used for indexing. The above also requires metadata at header/module level. By all means, the above is not feature complete, it is some idea on how it might work to have compile time guarantees...and if it looks suspiciously like Rust's lifetimes, I am sorry, I don't know Rust, any similarity is completely random. And perhaps this mechanism may be used for lifetimes as well, I don't know ;-).
jkbbwr 4y ago
You could potentially hit your codebase with CBMC using model assertions. Its basically what you already know in the TLA+ world but targeted against C++ code.
cjfd 4y ago
Well, being a C++ programmer for years who really would not mind going back to C++. Yes, these are known things. When one wants to call atoi, one has to know that either the string is zero terminated or that non-numerical characters follow the numerical ones... Same kind of thing with pop_back. I would not know that calling pop_back on an empty container is undefined behaviour by heart but I would be suspicious enough that it might be to check the docs to find out.
- roelschroeven 4y ago
  Putting string data in a vector<char> for starters is a big code smell. If you do, you're not using the language and the standard library the way they are supposed to be used and it's on you to make sure you do everything right. Then combining that with C-style string handling functions, without taking all the extra care, is just asking for trouble IMO.
cjbprime 4y ago
Nice quote from the previous post in this series: > Imagine you reached into a bag of chips only to find you’d already eaten the last one. However, your futilely grasping hand somehow triggers the incursion of every object in the known universe into the chip bag. Within nanoseconds a black hole forms and you are annihilated. This is roughly the experience of writing C++.
- jancsika 4y ago
  C developers: just don't reach back into the bag. It's pretty simple.ýýýý
- amrb 4y ago
  A bag of holding... in a bag of holding!
ksherlock 4y ago
level = 0; int32_t multiplier = 1; for (size_t i = 0; i < raw_level.size(); i++) { const size_t index = raw_level.size() - i - 1; const int8_t digit_value = raw_level.at(index) - 48; level += digit_value * multiplier; multiplier *= 10; } eek. i've always done something like: level = std::accumulate(raw_level.begin(), raw_level.end(), 0, [](int a, int c) { return a * 10 + (c - '0') } ); How does everyone else atoi when you can't atoi (and don't care about overflow etc)
TinkersW 4y ago
The pop_back() error would have been caught easily by an experienced C++ dev as common sense says run with assertions enabled in dev builds(With MSVC use _CONTAINER_DEBUG_LEVEL =1). The other error was just dumb using a C function when a good C++ alternative exists.
- Kranar 4y ago
  Don't know what your domain is, but most experienced devs in most of the domains I work in turn off those assertions because they slow the absolute hell out of debug builds. Like yes, debug builds are naturally slower, but there's slow and there's unbearable, and MSVC's debug checks are simply unbearable. GCC/clang both have a set of reasonable assertions that don't impact performance too much, so if possible I use those to do my testing.
  - TinkersW 4y ago
    No they don't, you are thinking of older implementations(the old checked iterators _ITERATOR_DEBUG_LEVEL etc in msvc). That shit was indeed very slow. You can run release builds with _CONTAINER_DEBUG_LEVEL =1 and the performance impact is minor.
batty_alex 4y ago
> for (size_t i = 0; i < raw_level.size(); i++) They say this is C++ code but, this is all C code. For instance, why aren't they using a range-based for loop? for (auto& level : raw_level) Boom! No more bugs. > I certainly don’t feel as safe writing this code as I would in rust Ah... there it is
- mr_00ff00 4y ago
  As this is HN, all trendy systems languages must be mentioned so here I go: Jokes aside, I think these are the types of errors I assume Carbon would be well suited to prevent and justify why Carbon is a great idea. Yes the code is C technically not C++, but the issue remains that it isn’t forced to be safer. The goal of languages like Carbon is to force these things in those large C++ repos.
- ddulaney 4y ago
  Unfortunately, it's also C++ code. Most of the weakest parts of C++ come from its close association with C. When asking why some people like Rust over C++, I think not enough weight is given here: Rust got to not worry about decades of legacy C stuff seeping in. If Rust isn't your jam -- and I get why it wouldn't be -- there are some initiatives starting now from within the C++ community to shed the C legacy with a new language that feels a lot more like modern C++. Herb Sutter's cppfront[0] and Carbon[1] are examples. But I don't think it makes sense to dismiss this criticism just because the author happens to like Rust. These C idioms continue to be valid and reasonably widely used in C++. [0]: https://github.com/hsutter/cppfront https://github.com/hsutter/cppfront [1]: https://github.com/carbon-language/carbon-lang https://github.com/carbon-language/carbon-lang
eklitzke 4y ago
I don't understand the comment in the post about how the vector including a large address space after .pop_back() could include private keys. Using the most literal interpretation of the post it sounds like the author doesn't understand how virtual memory works. You can try to access any memory address you want, but if it's not mapped the program will segfault, and your program address mappings don't allow you to access the memory of other programs (unless you're doing some kind of exotic memory sharing mechanism like SysV shared memory or using memfd on Linux, but both of those require a lot of coordination by both sides). This kind of memory protection exists even if the program is running as root. The more charitable interpretation is that he's alluding to some situation where your own program has private keys loaded in memory, and now that you've done some bad thing and gotten memory corruption you could leak/reveal the private keys. This is obviously true, but once you have a core dump or are debugging a process in GDB you have access to the entire program's address space anyway. And there are a ton of well known ways to protect against this situation: generally you have some small and well audited portion of the program handle the private key data and things like key exchange, and then you the rest of your code runs in a child/worker process that doesn't need access to the keys themselves. This is for example how OpenSSH and nginx work. This is sometimes called the "principle of least privilege" and is a best practice anyway for security-sensitive applications, even if they're written in a memory safe language like Rust.
BoorishBears 4y ago
ChatGPT finds the bugs... - First case is called out very strongly: > assume 'raw_level' is of the type 'std::vector<char>'. Is there a memory safety issue with the code 'int level = atoi(raw_level.data());'" Yes, there is a potential memory safety issue with the code int level = atoi(raw_level.data());. The std::vector<char> class does not guarantee that the elements stored in the vector are null-terminated, which is required by the atoi function. The atoi function expects a null-terminated string as its argument, and if the input string is not null-terminated, the behavior of atoi is undefined. Therefore, if raw_level does not contain a null-terminated string, this code may result in undefined behavior, including a crash or incorrect results. - Second case is lightly warned on: > find a memory safety bug in this code <code for handle_qed_keyword_token> The code you have provided does not appear to have any memory safety bugs. However, it depends on the context in which this code is used and the implementation of certain functions such as get_current_proof_level and pop_back. If these functions are not implemented correctly or if the code is used in an unintended way, there may be a potential for memory safety issues to arise. For example, if pop_back is called on an empty proofs vector, it can result in undefined behavior and potentially cause a crash or data corruption.

138 points by signa11 4y ago

deleted 4y ago

[deleted]

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.

zabzonk 4y ago

well, one of the problems could have been detected with "grep atoi source_code_here". any code that uses atoi is almost certainly bad (no error detection), and C++ provides several alternatives.

jeffbee 4y ago
Right? Why do people love these C library functions? I think the biggest problem in the C++ community is that some people know a little C. The memcpy of the vector’s data sounds like it also might be non idiomatic, but I don’t see the source code so I am not sure.
- gpderetta 4y ago
  Ideally the compiler would suggest to replace atoi with the C++ equivalent. But not even clang-tidy does with the set of warnings I have enabled.
  - pclmulqdq 4y ago
    That's not the philosophy of the C++ compiler writers. There are linters that suggest it, but compilers don't do this sort of warning.
- zabzonk 4y ago
  i don't even understand why atoi gets used in pure C code - there are much better standard C alternatives.
- eklitzke 4y ago
  In this case the code was using the .data() on a std::vector<char>, so it wouldn't have even been possible to use something like std::stoi (or something nicer like absl::SimpleAtoi) since the C++ routines need a std::string (or absl will accept std::string_view), not a C style string. Which in itself shows you that the code is problematic, because it's been written in a way where you can't even use idiomatic C++ library functions to do basic tasks. For the memcpy() thing the idiomatic way would have been to use std::copy() with the vectors begin/end iterators, and std::copy will do the right thing in this situation with a zero sized vector. The compiler will generate the same code with std::copy() as memcpy() anyway if the vector contains a POD type (as it did in this example), so there's not even a performance argument to be made for memcpy().

tubs 4y ago

LD_PRELOAD is probably want you want.

omnicognate 4y ago

Enabling libstdc++ debug mode (-D_GLIBCXX_DEBUG) would probably have located the second one, assuming gcc. It adds bounds and other checks to containers like std::vector. [1] Edit: The article mentions clang, so probably [2] is the relevant link. [1] https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_u... [2] https://libcxx.llvm.org/DesignDocs/DebugMode.html https://libcxx.llvm.org/DesignDocs/DebugMode.html

account42 4y ago
You can use libstdc++ with Clang (-stdlib=libstdc++) and libc++ with GCC (-nostdlib along with adding the libraries and include paths manually). In fact, libstdc++ tends to be the default for Clang installs on Linux for ABI-compatibility with GCC-compiled C++ libraries unless you pass -stdlib=libc++.

meindnoch 4y ago

Don't use atoi. Use std::from_chars.

account42 4y ago
This makes sense for integers but beware floating point from_chars - libc++ still doesn't implement it and libstdc++ implements it by wrapping locale-dependent libc functions which involves temporarily changing the thread locale and possibly memory allocation to make the passed string 0-terminated. IMO libstdc++'s checkbox "solution" is worse than not implementing it at all - user's are better off using Lemire's API-compatible fast_float implementation [0]. Of course it's possible that the author wanted to support compilers released before 2018 when that code was written in which case not even integer from_chars would be available. [0] https://github.com/fastfloat/fast_float https://github.com/fastfloat/fast_float
- meindnoch 4y ago
  Hm. Good to know! What about strtof?
  - zabzonk 4y ago
    the strtoX functions have been around for many, many years, and should be completely reliable. but i would use strtod. in fact i never use floats in my code unless i am dealing with very large numbers of floating-point values.
simiones 4y ago
Is there an alternative if not using C++17 or newer?
- zabzonk 4y ago
  several (or should that be many?) - for example stoi, sscanf, operator >>(istream &, int &) - there are others.
vitus 4y ago

viraptor 4y ago

I wonder if PVS would flag those issues. (https://pvs-studio.com/en/ https://pvs-studio.com/en/) I expect it to flag the atoi, but not sure about the pop.

jussij 4y ago

This is the beauty of modern C++. The code has become so hard to read (without years of prior knowledge) that as a human, it's rather hard to find any possible errors. The code can take on a Schrodinger state where it appears both right and wrong.

simplotek 4y ago
The code is not hard to read. People write code that is hard to read. English is a great language, but some people write stuff down in good plain English that is hard to parse. The same goes for any programming language. If you want to write code that's hard to read, that's what you will get.
- bArray 4y ago
  I don't not think that English isn't difficult to parse. I agree completely that it's all about the code you write. You can write nice C++ that is easy to understand, easy to debug, and that you can be quite certain is bug free. Something like Rust appears to eliminate one class of bugs, but then people still write bugs. Just look at the Rust language issue tracker on GitHub [1]. Fundamentally, you cannot get around the need for good code design. Any useful programming language will always have the ability to mishandle data. [1] https://github.com/rust-lang/rust/issues https://github.com/rust-lang/rust/issues
  - galangalalgol 4y ago
    Rust doesn't create any new classes of bugs, and it eliminates some old ones. Isn't that all positive? Rust also turns a lot more bugs from UB and/or exploitable, into a panic. That is still potentially a ddos, but it is better than code injection. And therebare tools like prusti to find panics, and you can use tricks with linking a nostd library to see if you got them all. Probably better ways than that now. We can obviously write bugs in any language, but it is just as obvious that some languages make finding some bugs easier. Rust makes some bugs easy to find, the compuler points them out, and unless it makes other bugs harder to find it is all upside (from the correctness standpoint, not all standpoints)

pornel 4y ago

Is there a C++ proposal to add mandatory bounds checks to methods like pop_back? If modern C++ is serious about getting memory safety, UB like this should be changed to have a run-time check and abort. This is even backwards compatible, cheap or zero cost — if it's used in a loop that checks for non-empty length, the condition could be propagated and the redundant check optimized out. I assume it won't get fixed, because from what I've seen so far if there is even slightest whiff of a performance overhead, modern C++ still chooses unsafety. If there's any inertia or backwards compatibility worry, even if it's a spacebar-heater, modern C++ still chooses to keep the old unsafety. Safety is still not a priority.

aw1621107 4y ago
At least based on this comment from the maintainer of the C++ NanoRanges library [0], it seems contracts (at one point) may be the intended method to bounds check: > I actually proposed at() for std::span as part of P1024r0. It was soundly defeated in LEWG, 0|0|4|4|5. > I wasn’t there for the meeting, but from what I understand the feeling was that contracts (which at that time seemed highly likely for ‘20) were a better way to go for bounds checking, and that we should not be throwing exceptions for programmer errors like out-of-bounds access. No idea whether that is still the case though. [0]: https://old.reddit.com/r/cpp/comments/djqdu2/why_is_stdspan_is_missing_at/f48hhb5/ https://old.reddit.com/r/cpp/comments/djqdu2/why_is_stdspan_...
- bluGill 4y ago
  I'm not sure what the current thought is, but I'm hopeful contracts makes it into C++, and over a the next years (and iterations of the language) we find ways to make C++ better by proving the whole program correct. This will need a lot of tool support, and changes in the way people write code. However mathematical proofs of the program are the only thing I have left to make my code more reliable: humans are not perfect and unit tests have limits (proofs have different limits).
  - WanderPanda 4y ago
    I don't entirely get the concept between mathematical proofs of programs. Is the basic idea that it is straight forward for humands to write down a correct NP hard specification and then let some algorithm proof the correctness of a more efficient algorithm for the same problem?

inetknght 4y ago

This post supports my personal experience. ASAN is a flippin' godsend. I have never encountered a false report from ASAN. I have seen it have false negatives but those are usually intermittent with true positives when the tests are run again and usually related to very small buffer overflows (eg, a "few bytes" past the end of a buffer). Not perfect but no false positives means it's 100% worthwhile to have enabled. But UBSAN is... not as helpful. It sometimes catches things. It often misses some undefined behavior that can be easily identified elsewhere. But the real kicker is that (almost?) all of the sanitizers are mutually exclusive. That really sucks if you're trying to debug something very complex. It also has lead me to build and drive the complexity into smaller and smaller chunks. Arguably that's a good thing! I have, for several years now, always turned Address Sanitizer on by default when -DCMAKE_BUILD_TYPE=Debug or off when it's not. I've added an option that forces the option with -DASAN_BUILD=bool. I wish Kitware would simplify my CMakeLists files by making this default behavior. And I think it would really help a lot of open source projects that aren't configured for ASAN too

cyber_kinetist 4y ago
I think there’s a place for Address Sanitizer to be enabled on RelWithDbgInfo build type (usually used for development and not production). Debug mode in C++ is already too slow, but adding a sanitizer makes it almost unusable for performance-sensitive code.
- kllrnohj 4y ago
  The sanitizers are not incompatible with optimizations. asan with -O2 is a perfectly valid & useful combination. It's much faster than valgrind or anything like that, very usable levels of performance. And on an increasingly common number of CPUs there's hardware acceleration for ASAN as well ( https://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html https://clang.llvm.org/docs/HardwareAssistedAddressSanitizer... & https://source.android.com/docs/security/test/hwasan https://source.android.com/docs/security/test/hwasan )
mannykannot 4y ago
With regard to UBSAN, are the misses that you see examples of specific checkers failing to find the specific sort of UB that they are intended to detect? If so, do the majority of misses seem to come from a small subset of the checkers?

wyldfire 4y ago

Another option for dynamic bounds checking on heap allocations is Electric Fence or scudo. It probably wouldn't find that issue with pop_front() though.

axilmar 4y ago

The problem of hidden types surfaces again. Obviously passing null to a function that does not expect null is a well known problem, tackled by many programming languages but not by C and C++. This is a purely typing issue, because null is a different thing than a valid memory address. The other issue about vector.pop_back() is also a hidden type issue...an empty vector is not the same as a non-empty vector, and therefore pop_back() applies only to non-empty vector. What would a possible solution be in the context of C++? Well, each function should only be applicable when the parameters are in the states the function expects. So parameters (including this/self) shall have states. For example: void pop_back() this : not_empty { ... } But this is not enough. The language should allow the user to specify change of states at certain statements/expressions. For example: void clear() { ... this := empty; } And then the compiler can check the AST if the used vector has the 'empty' state on pop back or not. Doing the following would be illegal: vector<int> data; data.clear(); data.pop_back(); And the following would be illegal because one execution path would lead to a state violation: vector<int> data; if (some_external_condition) { data.clear(); } data.pop_back(); And the following would be illegal too, because the result of clear_random_vector could be possible in empty state. vector<int>& clear_random_vector(vector<int>& a, vector<int>& b) { if (rand() < RAND_MAX / 2) { //c++ has better random number interface a.clear(); return a; } b.clear(); return b; } int main() { vector<int> a{1, 2, 3}, b{4, 5, 6}; vector<int>&v = clear_random_vector(a, b); v.pop_back(); } And in case the function clear_random_vector is in another translation unit, then: a) if headers are used, the programmer shall provide metadata about the state of the result in the header. Example: vector<int>& clear_random_vector(vector<int>& a, vector<int>& b) := empty; b) if modules are used, the compiler itself should output the metadata. Actually, this would also solve the null pointer problem, because a pointer would have the states empty, not_empty by default, and then functions could be like this: memcpy(void* dst : not_empty, const void* src : not_empty, size_t count); And then this would be caught at compile time: std::vector<char> dst, src; memcpy(dst.data(), src.data(), 10); States could also solve the problem of array indexing. For example, an index can have two states, 'in_range' and 'out_of_range'. Using an integer as an array index would implicitly add the two states in the index variable; comparing the integer with the array range would automatically add 'in_range' or 'out_of_range' to the integer. Example of valid usage: vector<int> data{1, 2, 3}; int i = 0; //our index //'i' gets automatically 'in_range' state. if (i < data.size()) { std::cout << data[i] << std::endl; } The following would be invalid, because straightforward array indexing would requre a state named 'in-range': vector<int> data{1, 2, 3}; int i = 0; //our index std::cout << data[i] << std::endl; How would that work at language level? well, here is an idea: a) any operation can affect the compile-time state of objects, b) states themselves would be templates, bound to certain ids at compile time. Here is a possible implementation: template <class T> class vector { //returns a 'size_t' variable tagged with state 'size_of_this' and parameter 'this'; in this case, this does not refer to the runtime address of the object, but to the identifier this object is bound to. size_t size() const : size_of_this<this> { ... } } //index is bound to state bound_index<T>, where T refers to the variable of the vector at compile time. template <T> operator < (int& index, size_t size: size_of_this<T>) { index := size_of_this<T>; } template <class T> class vector { //the vector operator [i] requires size_of_this<T> const T& operator [](int index : size_of_this<this>) const { ... } //adding an element to the vector breaks size_of_this. void push_back(const T& elem) { this := !size_of_this<this>; } } So, each time push_back is called on a vector, the guarantee about indexes breaks, and the index that previously holds the guarrantee can no longer be used for indexing. The above also requires metadata at header/module level. By all means, the above is not feature complete, it is some idea on how it might work to have compile time guarantees...and if it looks suspiciously like Rust's lifetimes, I am sorry, I don't know Rust, any similarity is completely random. And perhaps this mechanism may be used for lifetimes as well, I don't know ;-).

jkbbwr 4y ago

You could potentially hit your codebase with CBMC using model assertions. Its basically what you already know in the TLA+ world but targeted against C++ code.

cjfd 4y ago

Well, being a C++ programmer for years who really would not mind going back to C++. Yes, these are known things. When one wants to call atoi, one has to know that either the string is zero terminated or that non-numerical characters follow the numerical ones... Same kind of thing with pop_back. I would not know that calling pop_back on an empty container is undefined behaviour by heart but I would be suspicious enough that it might be to check the docs to find out.

roelschroeven 4y ago
Putting string data in a vector<char> for starters is a big code smell. If you do, you're not using the language and the standard library the way they are supposed to be used and it's on you to make sure you do everything right. Then combining that with C-style string handling functions, without taking all the extra care, is just asking for trouble IMO.

cjbprime 4y ago

Nice quote from the previous post in this series: > Imagine you reached into a bag of chips only to find you’d already eaten the last one. However, your futilely grasping hand somehow triggers the incursion of every object in the known universe into the chip bag. Within nanoseconds a black hole forms and you are annihilated. This is roughly the experience of writing C++.

jancsika 4y ago
C developers: just don't reach back into the bag. It's pretty simple.ýýýý
amrb 4y ago
A bag of holding... in a bag of holding!

ksherlock 4y ago

level = 0; int32_t multiplier = 1; for (size_t i = 0; i < raw_level.size(); i++) { const size_t index = raw_level.size() - i - 1; const int8_t digit_value = raw_level.at(index) - 48; level += digit_value * multiplier; multiplier *= 10; } eek. i've always done something like: level = std::accumulate(raw_level.begin(), raw_level.end(), 0, [](int a, int c) { return a * 10 + (c - '0') } ); How does everyone else atoi when you can't atoi (and don't care about overflow etc)

TinkersW 4y ago

The pop_back() error would have been caught easily by an experienced C++ dev as common sense says run with assertions enabled in dev builds(With MSVC use _CONTAINER_DEBUG_LEVEL =1). The other error was just dumb using a C function when a good C++ alternative exists.

Kranar 4y ago
Don't know what your domain is, but most experienced devs in most of the domains I work in turn off those assertions because they slow the absolute hell out of debug builds. Like yes, debug builds are naturally slower, but there's slow and there's unbearable, and MSVC's debug checks are simply unbearable. GCC/clang both have a set of reasonable assertions that don't impact performance too much, so if possible I use those to do my testing.
- TinkersW 4y ago
  No they don't, you are thinking of older implementations(the old checked iterators _ITERATOR_DEBUG_LEVEL etc in msvc). That shit was indeed very slow. You can run release builds with _CONTAINER_DEBUG_LEVEL =1 and the performance impact is minor.

batty_alex 4y ago

> for (size_t i = 0; i < raw_level.size(); i++) They say this is C++ code but, this is all C code. For instance, why aren't they using a range-based for loop? for (auto& level : raw_level) Boom! No more bugs. > I certainly don’t feel as safe writing this code as I would in rust Ah... there it is

mr_00ff00 4y ago
As this is HN, all trendy systems languages must be mentioned so here I go: Jokes aside, I think these are the types of errors I assume Carbon would be well suited to prevent and justify why Carbon is a great idea. Yes the code is C technically not C++, but the issue remains that it isn’t forced to be safer. The goal of languages like Carbon is to force these things in those large C++ repos.
ddulaney 4y ago
Unfortunately, it's also C++ code. Most of the weakest parts of C++ come from its close association with C. When asking why some people like Rust over C++, I think not enough weight is given here: Rust got to not worry about decades of legacy C stuff seeping in. If Rust isn't your jam -- and I get why it wouldn't be -- there are some initiatives starting now from within the C++ community to shed the C legacy with a new language that feels a lot more like modern C++. Herb Sutter's cppfront[0] and Carbon[1] are examples. But I don't think it makes sense to dismiss this criticism just because the author happens to like Rust. These C idioms continue to be valid and reasonably widely used in C++. [0]: https://github.com/hsutter/cppfront https://github.com/hsutter/cppfront [1]: https://github.com/carbon-language/carbon-lang https://github.com/carbon-language/carbon-lang

eklitzke 4y ago

I don't understand the comment in the post about how the vector including a large address space after .pop_back() could include private keys. Using the most literal interpretation of the post it sounds like the author doesn't understand how virtual memory works. You can try to access any memory address you want, but if it's not mapped the program will segfault, and your program address mappings don't allow you to access the memory of other programs (unless you're doing some kind of exotic memory sharing mechanism like SysV shared memory or using memfd on Linux, but both of those require a lot of coordination by both sides). This kind of memory protection exists even if the program is running as root. The more charitable interpretation is that he's alluding to some situation where your own program has private keys loaded in memory, and now that you've done some bad thing and gotten memory corruption you could leak/reveal the private keys. This is obviously true, but once you have a core dump or are debugging a process in GDB you have access to the entire program's address space anyway. And there are a ton of well known ways to protect against this situation: generally you have some small and well audited portion of the program handle the private key data and things like key exchange, and then you the rest of your code runs in a child/worker process that doesn't need access to the keys themselves. This is for example how OpenSSH and nginx work. This is sometimes called the "principle of least privilege" and is a best practice anyway for security-sensitive applications, even if they're written in a memory safe language like Rust.

BoorishBears 4y ago

ChatGPT finds the bugs... - First case is called out very strongly: > assume 'raw_level' is of the type 'std::vector<char>'. Is there a memory safety issue with the code 'int level = atoi(raw_level.data());'" Yes, there is a potential memory safety issue with the code int level = atoi(raw_level.data());. The std::vector<char> class does not guarantee that the elements stored in the vector are null-terminated, which is required by the atoi function. The atoi function expects a null-terminated string as its argument, and if the input string is not null-terminated, the behavior of atoi is undefined. Therefore, if raw_level does not contain a null-terminated string, this code may result in undefined behavior, including a crash or incorrect results. - Second case is lightly warned on: > find a memory safety bug in this code <code for handle_qed_keyword_token> The code you have provided does not appear to have any memory safety bugs. However, it depends on the context in which this code is used and the implementation of certain functions such as get_current_proof_level and pop_back. If these functions are not implemented correctly or if the code is used in an unintended way, there may be a potential for memory safety issues to arise. For example, if pop_back is called on an empty proofs vector, it can result in undefined behavior and potentially cause a crash or data corruption.