Remix.run Logo
moralestapia 8 months ago

[flagged]

klempner 8 months ago | parent | next [-]

The idea that RAII covers "99% of your ass" is "the low-IQ level statement".

Temporal safety is the primary hard problem from a memory safety standpoint, and RAII does nothing to solve it at least the moment a memory allocation crosses abstraction boundaries.

wigglyartichoke 7 months ago | parent | next [-]

What's an example? I'm just a hobbyist when it comes to c++

dbremner 7 months ago | parent [-]

Here is a real safety issue that I found and fixed a couple weeks ago. This is an ancient language runtime which originally ran on MS-DOS, Amiga, Atari, and at least a dozen now-discontinued commercial Unices. I've been porting it to 64-bit OSes as a weekend hack. While this particular issue is unlikely to appear in modern applications, a similar pattern might manifest today as a use-after-free error with std::string_view or an invalid iterator.

Background:

    typedef int Text; 
The Text type is used to encode different kinds of string-like values. Negative values represent a generated dictionary. Valid indexes in the string intern table (https://en.wikipedia.org/wiki/String_interning) represent a stored string. Other values represent generated variable names.

const char *textToStr(Text t) - This takes a Text value and returns a pointer to a null-terminated string. If t is a string intern index, then it returns a pointer to the stored string. If t represents either a generated dictionary or generated variable name, then it calls snprintf on a static buffer and returns the buffer's address.

Problem:

The use of a static buffer in textToStr introduces a temporal safety issue when multiple calls are made in the same statement. Here’s an excerpt from a diagnostic error message, simplified for clarity:

    printf(stderr, "Invalid use of \"%s\" with \"%s\"",
                textToStr(e),
                textToStr(s));
If both e and s are generated dictionaries or variables, then each call to textToStr overwrites the static buffer used by the other. Since the evaluation order of function arguments in C++ is undefined, the result is unpredictable and depends on the compiler and runtime.
scintill76 7 months ago | parent [-]

If you're saying C++ won't automatically save you from design mistakes, then I agree. This is a poorly-specified function. The caller can't know exactly how they need to handle the returned pointer.

Potential solutions:

* Return std::string and eat the cost of sometimes copying interned strings

* std::string with your own allocator could probably deal with the two cases fairly cheaply and transparently to the caller

* overload an operator<< or similar and put your conversion result directly into a stream, rather than a buffer that then goes into a stream

* put your generated values in a container that can keep pointers into it valid under all the operations you do on it (I think STL sets would work), keep them there for the rest of the lifetime of the program, and return your pointers to them (or to the interned constant strings)

I think many of these could be termed RAII, so I lean toward the idea in this subthread that RAII and other C++ idioms will help you stay safe, if you use them.

P.S. The function is also not safe if called by multiple threads concurrently. Maybe thread-local storage could be an easy fix for that part, but the other solutions should fix it too. [But if you have any shared storage that caches the generated values such as the last solution, it needs to be protected from concurrency.]

dbremner 7 months ago | parent [-]

This is a 30+ year-old codebase that originally started as pre-ANSI C. The design tradeoffs were very different at the time. I've managed to compile it as C++23 with unreasonably high warning levels, and it's now warning-free. However, there are still many unresolved issues.

One notable limitation is the use of a SIGINT handler implemented with setjmp/longjmp, which reduces the effectiveness of RAII.

Regarding textToStr, there were four instances where it was called twice in the same expression. To avoid potential problems, I replaced these calls with a temporary std::string to store the result of the first call before the second is made. In hindsight, none of these cases likely would have caused issues, but I preferred not to take risks. The SigIntGuard ensures that the std::string destructor runs even if a SIGINT occurs:

    {
        SigIntGuard guard;
        std::string const member{textToView(textOf(fst(b)))};
        std::print(errorStream, R"(No member "{}" in class "{}")", member, textToView(klass(c).text));
    }
moralestapia 8 months ago | parent | prev [-]

No, if you have temporal safety issues you didn't understand RAII. That is pretty much the whole point of RAII.

AnimalMuppet 8 months ago | parent [-]

If you want anyone to believe you, you're going to have to give more than just a blank assertion. Can you give at least a sketch of your reason for your claim?

moralestapia 8 months ago | parent [-]

Reasoning is, if your objects outlive the scope of your class, then they most likely belong to a class that's higher in the hierarchy (they already do, de facto).

SoothingSorbet 7 months ago | parent [-]

Please explain how you would solve the iterator invalidation problem using only C++ and RAII. Thanks.

moralestapia 7 months ago | parent [-]

This small thread is about temporal safety so you're out of luck.

genrilz 7 months ago | parent [-]

One, iterator invalidation can be a temporal safety problem. Specifically, if you have an iterator into a vector and you insert into the same vector, the vector might reallocate, resulting in the iterator pointing into invalid memory.

Two, consider the unique_ptr example then. Perhaps a library takes a reference to the contents of the unique_ptr to do some calculation. (by passing the reference into a function) Later, someone comes along and sticks a call to std::async inside the calculation function for better latency. Depending on the launch policy, this will result in a use after free either if the future is first used after the unique_ptr is dead, or if the thread the future is running on does not run until after the unique_ptr is dead.

EDIT: Originally I was just thinking of the person who inserted the std::async as being an idiot, but consider that the function might have been templated, with the implicit assumption that you would pass by value.

sirwhinesalot 7 months ago | parent | prev | next [-]

Every study on security vulnerabilities has shown that "just don't screw up bro" doesn't scale.

Even if we ignore the absolute clown move of having no bounds checks by default (and std::span doesn't have them at all), it's very easy to get into trouble with anything involving C++ iterators and references.

moralestapia 7 months ago | parent [-]

>Every study on security vulnerabilities has shown that "just don't screw up bro" doesn't scale.

Let's see them.

sirwhinesalot 7 months ago | parent [-]

Well, ignoring all the reports from google, microsoft and mozilla, all of whom are part of a cabal spreading misinformation on the percentage of vulnerabilities caused by memory unsafety in C++ (all 3 arrived at around 70% so it's clearly a made up number they colluded to spread), and ignoring the reports from the United States government (probably infiltrated by rust cultists), I can recommend the paper Memory Errors: The Past, the Present, and the Future

ModernMech 7 months ago | parent [-]

Are you being sarcastic? I genuinely cannot tell.

sirwhinesalot 7 months ago | parent [-]

Yes, except for the legitimate paper recommendation.

adgjlsfhk1 8 months ago | parent | prev | next [-]

RAII only helps with 1 of 4 primary cases of safety. RAII deals (badly) with temporal safety, but not spacial safety (bounds errors etc), safe initialization (use before initialization), or undefined behavior (overflow/underflow, aliasing, etc).

badmintonbaseba 7 months ago | parent | next [-]

Use-after-free (or reference/iterator invalidation in general) is the main issue. RAII doesn't help there at all. RAII helps with deterministically cleaning up resources, which is important, but barely related to safety.

AnimalMuppet 8 months ago | parent | prev | next [-]

How does RAII not help with safe initialization? It's right in the name.

moralestapia 8 months ago | parent | prev [-]

>RAII deals (badly) with temporal safety

>safe initialization (use before initialization)

These two are solved by proper use of RAII.

But you have a point with UB. That's always been an issue, though, it's part of the idiosyncrasies of C/C++; all languages have their equivalent of UB.

sapiogram 7 months ago | parent | prev | next [-]

> equivalent to people unable to grasp type coercion on JS and thus blaming the language for it (literally just use '===' and stop bitching about it).

They're not even remotely equivalent. A single eslint rule has immediately and permanently fixed this in every Javascript project I've worked on, both for me and my coworkers' code. RAII helps, but in C++, no amount of linters and language features can fully protect you.

charleslmunger 7 months ago | parent | prev | next [-]

As a relative newcomer to C++, I have found RAII to be fine for writing in object-oriented style. But I would only subject myself to the suffering and complexity of C++ if I really wanted excellent performance, and while RAII does not generally have a runtime cost by itself, engineering for full performance tends to exclude the things that RAII makes easy. If you manage memory via arenas, you want to make your types trivially destructible. If you don't use exceptions, then RAII is not necessary to ensure cleanup. In addition, use of destructors tends towards template APIs that generate lots of duplicate code, when an API that used a C-style function pointer for generic disposal would have produced much smaller code.

And C++'s object model can add additional complexity and sources of UB. In C++20 previously valid code that reads a trivially destructible thread_local after it has been destroyed became UB, even though nothing has actually happened to the backing storage yet.

rerdavies 7 months ago | parent [-]

As an old-timer, I think you have some serious misconception about how RAII works, and what it does for you.

> Arena management

There's nothing that stops you from using arena allocators in C++. (See pmr allocators in C++17 for handling complex non-POD types).

> The cost of RAII_

you're going to have to clean up one way or another. RAII can be zero-overhead, and usually generates less code than the C idiom of "goto Cleanup".

> Use of destructors leads toward template APIs.

Not getting that. Use of destructors leads to use of destructors. Not much else.

> If you don't use exceptions....

Why on earth would you not use exceptions? Proper error handling in C is a complete nightmare.

But even if you don't, lifetime management is a huge problem in C. Not at all trivial to clean things up when you're done. Debugging memory leaks in C code was always a nightmare. The only thing worse was debugging wild memory writes. C++ RAII: very difficult to leak things (impossible, if you're doing it right, which isn't hard), and if it ever does happen almost always related to using C apis that should have been properly wrapped with RAII in the first place.

Granted, wrapping C handles in RAII was a bit tedious in C++89; but C++17 now allows you to write a really tidy AutoClose template for doing RAII close/free of C library pointers now. Not in the standard library, but really easy to roll your own:

    // call snd_pcm_close when the variable goes out of close.
    using snd_pcm_T = pipedal::AutoClose<snd_pcm_t*,snd_pcm_close>;

    snd_pcm_T pcm_handle = snd_pcm_open(....);

> C++ 20 undefined behavior of a read-after-free problem.

That's not UB; that's a serious bug. And C's behavior would also be "UB" if you read after freeing a pointer.

charleslmunger 7 months ago | parent | next [-]

>As an old-timer, I think you have some serious misconception about how RAII works, and what it does for you.

I appreciate the education :-)

>There's nothing that stops you from using arena allocators in C++.

This is true, but arenas have two wonderful properties - if your only disposable resource is memory, you don't need to implement disposal at all; and you can free large numbers of individual allocations in constant time for immediate reuse. RAII doesn't help for either of these cases, right?

>Use of destructors leads to use of destructors

I guess what I mean is... It's totally possible and common to have a zillion copies of std::vector in your binary, even though the basic functionality for trivially copyable trivially destructible types is identical and could be serviced from the same implementation, parameterized only on size and alignment. Destruction could be handled with a function pointer. But part of the reason templates are used so heavily seems to be that there's an expectation that libraries should handle types with destructors as the common case.

>lifetime management is a huge problem in C. Not at all trivial to clean things up when you're done.

Absolutely true if you're linking pairs of many malloc and free calls. But if you have a model where a per-frame or per-request or per-operation arena is used for all allocations with the same lifetime, you don't have this problem.

>And C's behavior would also be "UB" if you read after freeing a pointer.

The specific issue I ran into was the destructor of one thread_local reading the value of another thread_local. In C++17 the way to do this was to make one of them trivially destructible, as storage for thread locals is released after all code on that thread has finished, and the lifetime for trivially destructible types ends when storage is freed. In C++20 this was changed, such that the lifetime of a thread local ends when destroyed (rather than when storage is freed) if it's trivially destructible. C thread local lifetimes are tied to storage only and don't have this problem.

badmintonbaseba 7 months ago | parent | prev [-]

You can use `unique_ptr` with a custom deleter for wrapping C libraries.

  using snd_pcm_T = std::unique_ptr<
    snd_pcm_t,
    decltype([](void* ptr){snd_pcm_close(ptr);})
  >;
  auto pcm_handle = snd_pcm_T(snd_pcm_open(...));
The lambda in decltype is C++20, otherwise the deleter type could be declared out-of-line.
7 months ago | parent | prev | next [-]
[deleted]
binary132 8 months ago | parent | prev [-]

Just give up, they’ll never get it.

throw16180339 7 months ago | parent [-]

What's to get? It's an unsupported claim with substantial counterexamples in the form of every large C++ project. If everyone in the world gets RAII wrong then it doesn't matter what it's theoretically capable of.

moralestapia 7 months ago | parent [-]

Sure pal, in an imaginary world where:

Apache/nginx don't exist,

Chrome/V8 don't exist,

Firefox doesn't exist,

GCC/Clang don't exist,

MySQL doesn't exist,

TensorFlow doesn't exist,

VLC doesn't exist,

and the list goes on and on ...

Philpax 7 months ago | parent [-]

every single one of those has had exploits

moralestapia 7 months ago | parent [-]

Did I say they don't?

All software has bugs.

Is this supposed to be news?

genrilz 7 months ago | parent [-]

Problem with memory corruption as a bug is that unlike most classes of bug, memory corruption allows remote code execution. (see return oriented programming for the basic version, block oriented programming for the more complex version that bypasses most (all?) mitigation strategies) There are other types of bug that allow remote code execution like this, such as SQL or command-line injection, but those can be solved with better libraries.* However, memory management requires a strong enough type system in the language.

* Sorta, for command-line injection you have to know the way the command you are using processes flags and environmental variables in order to know that the filtering you are doing will work. It is absolutely better to use a library instead if you can get away with it.