Problem with c_resource

[melg8]

Preamble

Hi, @DanielaE I was really enjoying your presentation "Contemporary C++ in Action" especially when you talked about using C libraries and wrapping them up to make them more safe to use. After that i checked out this source code to figure out how it actually works. Now i want to:

1. Report possible bug
2. Spin some discussion about c_resource and/or wrapping C libs in general.

Bug part

Assumptions

1. This application code, although is somewhat "small example" still should handle error case scenarios.
2. c_resource assumed to be used in general case, where construct function (from C library) for resource allocation may fail.
3. Such failures, especially allocation failures assumed to be handled properly and not cause any undefined behavior.

Potential bug in error handling

CppInAction/Demo-App/videodecoder.cpp

Lines 134 to 139 in a669a1d

	libav::Packet Packet;
	libav::Frame Frame;
	int Result = 0;
	while (not atEndOfFile(Result) and successful(av_read_frame(File, Packet))) {
	const auto PacketReferenceGuard = Packet.dropReference();

• If av_packet_alloc function returns NULL because of failure (and it can do that, the documentation is explicit about it:"An AVFrame filled with default values or NULL on failure."
• Packet object with inner tPacket type object will have null pointer as ptr_ field
• Packet object will transfer this null value pointer by cast operator constexpr operator U *(this Self && self) when calling av_read_frame (File, Packet)
• This will trigger null pointer de-reference inside of library code.
• Because inside of library it calls read_from_packet_buffer - which de-references a pointer without check for null, or it will try to access to the field of this structure, without checking for null in else branch.

I assume it should have had same kind of check as

CppInAction/Demo-App/videodecoder.cpp

Line 168 in a669a1d

if (have(Decoder)) {

but for libav::Packet and maybe even for libav::Frame in decodeFrames function.

And if this really a bug, and I'm not wrong in my assumptions:

• It's sneaky
• this kind of mistake would be not uncommon and have noticeable impact on server side stability/correctness if applied at scale, exposing some room for exploitation
• It would be hiding in corners sometimes - when streams of error cases are mixed with streams of "filter by property" cases, and not exposing itself - until some zeroday.

Suggestions

If we talking not about general rethinking of c_resource approach, but fixing what we have in this particular case i would suggest.

1. Add customization point (macro or link time or template) when building for "test" purpose - not just call allocation function each time, but inserting NULL returns as failure on purpose. Test final application behavior with -fsanitize=address,undefined flags, test application under valgrind, providing different frequency of such allocation errors, from all of them failing, and till one in N calls failing.
2. Having in mind all cases found by previous step and by manual inspection of all usages of c_resource, add proper calls to have(resource) so it is in valid state before it used. Repeat step 1 to see changes.
3. Make interface of c_resource more thin, not allowing usage of de-reference/pointer acquisition when inner state is NULL without triggering assertion. Can be achieved with just addition of regular asserts, or by using not_null pointer type wrapper in places.
4. Unit tests? fuzzing of inputs? Maybe mutation based testing?
5. Maybe add some caveats/warnings about state of such implementation in readme/slides/comments.

Wrapping C API discussion part

For sake of usability c_resource template class is accumulating at least 3 different reasons to "have" or don't "have" inner (pointer) in it:

• The first is success/failure of construct function.
• The second is move semantics.
• The third is programmers good/evil will.

No doubt usability really shines, when you can just do this kind of manipulations:

CppInAction/Demo-App/videodecoder.cpp

Lines 161 to 166 in a669a1d

	auto makeFrames(fs::path Directory) -> std::generator<video::Frame> {
	auto PreprocessedMediaFiles = InfinitePathSource(std::move(Directory))
	| vws::filter(hasExtension(".gif"))
	| vws::transform(tryOpenAsGIF)
	| vws::transform(tryOpenVideoDecoder)
	;

But i think it only works on somewhat small scale example, which doesn't bother to separate reasons why its internal pointer is not there anymore.
Is it because:

1. Path is empty:

   CppInAction/Demo-App/videodecoder.cpp

   Line 80 in a669a1d

   if (not Path.empty() and

2. Or because emplace failed (and why it failed)
   

   CppInAction/Demo-App/videodecoder.cpp

   Line 81 in a669a1d

   successful(File.emplace(Filename.c_str(), nullptr, nullptr))) {

3. Or because we didn't find any GIFS there or we found one, but failed because of something else?
   

   CppInAction/Demo-App/videodecoder.cpp

   Line 82 in a669a1d

   File = acceptOnlyGIF(std::move(File));

So it's breaking single responsibility principle for code. It clamps down 3 different channels of "why" question into 1 bit of informational answer "have or doesn't have pointer value inside". At glance it looks like boost::outcome or std::optional, std::unique_ptr and gsl::not_null all married together inside of this implementation.

Because access to underlying pointer is not guarded by thin interface with preconditions and invartiants, like when using assertions, bugs can propagate through system unnoticed. While mixing semantically meaning of error case with filter case will add more problems debugging or even noticing logic errors or unhandled errors.

So in "easy to use, hard to misuse" - it definitely has "easy to use" part, but hard to misuse, and widening of error case channel of information (without big reworks of code), i think still somewhat lacking.

What are your thoughts? Do you have any suggestions about good resources/presentations/examples/practices for programmers trying to wrap C libraries in contemporary C++ at scale and with correctness and performance in mind? Thank you for your time!