Fix: Rule of 3/5/7 violation leads to possible use after free in inadvertent use of copy constructor /assignment.

[geo-ant]

First of all, thank you for this fantastic project. I've been using it at work and I've ran into a problem that took me longer to debug than I'm willing to admit 😅

Context

The problem manifested because I was returning a mem_outfile from a function with a signature like this:

ojph::mem_outfile encode_j2c_to_mem(uint8_t * data, uint32_t width, uint32_t height) {
   ojph::mem_outfile mf;
   // ... encode image and
   return mf;
}

This function could throw, so I was using it in a narrowly scoped try block like so:

ojph mem_outfile mf;

try {
   mf = encode_j2c_to_mem(data, width, height);
} catch (std::exception &e) {
   // error handling
}

// proceed using mf

What happens here is the following.

Root Cause

1. the temporary function return value is constructed
2. the copy-assignment operator of instance mf is invoked. The compiler will auto-generate this special function for us, which will perform a shallow copy.
3. The temporary is destructed, which will free the pointed-to buffer. That's the problem: the mf instance points to the same memory.
4. Once we try to use mf for output, we provoke UB, if we're lucky we segfault.

Godbolt Link: Here's a demo of the behavior on Godbolt.

Interlude: When it Works

I admit, that the more common use case would be to have the try block enclose the statement, such that we'd write it like so:

ojph mem_outfile mf  = encode_j2c_to_mem(data, width, height);

and then go on to use mf. This won't trigger the use after free. I think because NRVO kicks in and the destructor of the temporary isn't called in this case.

Fixes

To fix this behavior, I did the following:

1. Delete copy constructor and copy assignment. If this exists, it should perform a deep copy. I don't think this needs to exist here.
2. Create move constructor and move assignment. Since the moved-from values should be left in a valid state, I gave them the only valid state that always exists, which is the same as if they were default constructed. This nulls the pointer to the internal buffer, such that the destructor doesn't free the used memory.

[geo-ant]

Hi, just checking in to see if there's anything I can do to get this merged.

[aous72]

Hi, just checking in to see if there's anything I can do to get this merged.

Hi Geo-Ant,

Thank you for putting this in.

I am so sorry; my work conditions has changed lately. I fine with most of that you did. I need to do some touch up. Perhaps you can do them. I will leave you comments in the commit, or perhaps I can modify the code myself.

Kind regards,
Aous.

[aous72]

This is missing the test for self-move. It just needs to test for that case.

[geo-ant]

Good catch!

[aous72]

In general, I try to avoid many C++ stuff.
The std::swap here can be replaced by a simple copy because all the types are simple types.
Who knows what the compiler does with these.

Then rhs should be initialized to initial state. For this initial state, I think we need to define a private function void init() that initializes all the variables, and call this private function from the default constructor, the move constructor, and the move = operator.

I will do these once I have the time. Alternatively, you can do them, and let me know.

[geo-ant]

That sounds sensible to me. I'll take care of it.

[aous72]

First of all, thank you for this fantastic project. I've been using it at work and I've ran into a problem that took me longer to debug than I'm willing to admit 😅

Thank you for your encouraging words, and for looking into my code.

[aous72]

The problem manifested because I was returning a mem_outfile from a function with a signature like this

Is this necessary? or is there a simpler and better way? It is fair to ask this question.

[geo-ant]

Hi, just checking in to see if there's anything I can do to get this merged.

Hi Geo-Ant,

Thank you for putting this in.

I am so sorry; my work conditions has changed lately. I fine with most of that you did. I need to do some touch up. Perhaps you can do them. I will leave you comments in the commit, or perhaps I can modify the code myself.

Kind regards,

Aous.

No worries at all! Thank you for taking a look, I'm happy to work on the review comments (some time this week hopefully)

[geo-ant]

The problem manifested because I was returning a mem_outfile from a function with a signature like this

Is this necessary? or is there a simpler and better way? It is fair to ask this question.

Absolutely fair question. I think there are two ways to answer this question:

The first question is whether it's stricyly necessary to be able to return a mem_outfile. And the answer is probably no, it's not necessary for the functionality of the library. It just makes some use cases very convenient. TBH I don't remember my exact use case, I'd have to look at exactly why I wanted to do that. I was exploring an application to encode and decode images rapidly to check the influence of different quality levels.

I would still argue that the current behaviour is not acceptable because it can be done and it will invoke the copy assignment operator which leads to UB.

So a solution could also be to delete those special functions (copy/move construction and assignment). I'm not a CPP standards lawyer anymore but I think this would mean that we can still return an instance from a function as long as (N)RVO is used. That might depend on the CPP standard used. So you'll effectively allow it for some CPP standards and forbid it for others. Exposing the move special functions exposes a much more backward compatible way of making the same code reusable across Cpp versions. Again, I'm not 100% confident in this, maybe (N)RVO doesn't change that much across standards. Anyway, that would be my argument for my argument for the proposed solution.

EDIT: it's also important to note that any invocation of the copy constructor or assignment will exhibit this UB. The usecase I mentioned is just how I ran into the problem.

[geo-ant]

I've addressed the review comments. I've followed your advice and removed the use of std::swap. Rather than an init function I opted to implement a reset function that we can use in the move assignment and default construction. I've also implemented move construction using move assignment to reduce code duplication. I know people have different opinions on this and I'm completely fine changing the implementation. I just opted for the least amount of repetition.

[aous72]

Thank you Geo-ant. I am actually not very familiar with move methods. I do not use them. and if I had a choice I would delete them.

[geo-ant]

Thank you Geo-ant. I am actually not very familiar with move methods. I do not use them. and if I had a choice I would delete them.

Thank you for taking the time to review and merge this! And thank you again for this library.