Fix ABI incompatibility when calling a C++ compiled lodepng.cpp from C code or vice versa

[bobsayshilol]

The addition of a virtual destructor to LodePNGState when compiled as C++ causes a vptr to be added at the start of the struct. This results in the members of the struct changing their offset depending on whether they're seen from C or C++ code. Internally the LodePNGState struct is never deleted (as to invoke the virtual destructor) and has no virtual methods called on it so it doesn't need to be virtual. Hence fix the ABI incompatibility by removing the virtual destructor.

A demonstration of the problem using Compiler Explorer: https://godbolt.org/z/UJHBLf. Note that when compiled as C++ the offset of error moves 8B further into the struct due to the pointer inserted at the beginning.

Dropping the virtual destructor for LodePNGState also allowed the dropping of the virtual keyword from lodepng::State::~State() as described in the commit message.

[lvandeve]

Interesting suggestion.

The virtual keyword is added in both in case of C++ to provide safe C++ programming in case somebody extends the state with further subclasses and/or uses new and delete and mixes the LodePNGState and lodepng::State types (even though it's not necessary to do those things).

The C vs C++ ABI incompatibility is an interesting point. The struct is not intended to have one guaranteed fixed order or memory offset of its fields though: future versions could insert a new field at the beginning or in the middle without indicating this as a "backwards incompatible" change.

What is an example situation where different offsets of the fields in the C and C++ version cause a problem, and is the problem compiler errors, crashes, or something else? Can adding extern "C" in a correct place fix it?

Knowing the severity of the issue (e.g. if this makes it impossible to mix C and C++ at all) can help trade it off against the "safe C++ usage" point above.

Thank you :)

[bobsayshilol]

So initially I was going to suggest that name mangling in C++ will avoid this issue in most places (ie you could compile lodepng.c and lodepng.cpp in the same project and then have the mangling pick the correct one, though the client of LodePNG would still have to be careful about not passing LodePNGState's between their C and C++ code), however due to things like lodepng_default_decompress_settings it would be an ODR violation to have two instances of the loadpng implementation when it came to linking.

This means that the implementation would need to be either C or C++ (can't have one of each), so if the caller wasn't using the same language as the implementation (ie calling into a C++ loadpng.cpp from a C some_other_file.c) then the struct sizes wouldn't match and you'd get undefined behaviour.

Here's an example assuming that the implementation was compiled as lodepng.c:

//
// c_header.h
//
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

/* Needed for LodePNGTime */
#include "lodepng.h"

void some_c_function(void);
void another_c_function(LodePNGTime*);
/* ... */

//
// cpp_file.cpp
//
/*
 * This is a C header we're including, so make it C linkage.
 * This is how Lua recommends using its headers for example.
 * The C header may also wrap itself in extern "C" behind a
 * __cplusplus check.
 */
extern "C"
{
  /*
   * Everything in "c_header.h" is now seen as C linkage, including
   * the LodePNG API.
   */
  #include "c_header.h"
}

void DoSomething()
{
  /*
   * This state has a vptr in it because even though the header was
   * in an extern "C" block the __cplusplus macro was still defined.
   */
  LodePNGState * state = new LodePNGState;

  /*
   * This will call the C version of the API which expects a struct
   * without the vptr, trampling it in the process.
   */
  lodepng_state_init(&state);

  /* ... */

  /*
   * Without optimisations this will read the vptr to work out the
   * correct destructor, however since it was trampled in lodepng_state_init()
   * this could jump anywhere.
   */
   delete state;
}

I'm unconvinced about using the virtual to extend the class, since most clients will more likely add the LodePNGState object as a member of a bigger class than derive from it because the client can't extend it in a way that makes LodePNG's API act any differently. Deleting it through a pointer to the base class is also something I also wouldn't expect to see, again due to it being a member, though I'm happy to be proven wrong (the above example was just to show the undefined behaviour when calling the destructor since the stack would likely be a more appropriate location for it than the heap in that example).

Also it might be worth wrapping the non-C++ parts of the API in an extern "C" block anyway so that all code sees the non-C++ parts of the API as unmangled, avoiding linker errors when C++ code tries to link against a C implementation (or vice versa).

[bobsayshilol]

Ping as it's been a while :P

So looking back over this I guess the problem boils down to places where C and C++ code can interact since they'll have different views of LodePNGState.

Due to name mangling rules it wouldn't be possible for C++ code to call a C-compiled lodepng (and vice versa) unless the C++ code sees the lodepng headers through an extern "C" block as mentioned previously. Note that the reverse isn't true since C code will never mangle to be like C++ (AFAIK). Fortunately variable names are mangled like C in C++ which means that you can't have more than one lodepng of either language in an application since global variables like LODEPNG_VERSION_STRING would (hopefully) give linker errors due to ODR were that the case.

There's also the case where a LodePNGState may be used in structs, which would alter their sizes if they were passed between C and C++. For example:

struct some_c_state {
  LodePNGState state;
  char const * name;
  unsigned flags;
};

would be bigger in C++ and the flags member would be seen in a different place between C and C++, leading to interesting behaviour when accessed.

So as an example let's say that you're working with a mate on something, maybe a game. They're using C++ and you're using C. You start using lodepng and they decide they want to too for some simple image loading in their parts of the game. So they wrap the headers that you've been using in extern "C" and everything links and runs as expected because they're only using lodepng_decode_memory() at the moment. Later down the road they start wanting to load more complex images and start using lodepng_decode() and suddenly you're both getting random crashes that don't make any sense, or worse, strange and hard to diagnose behaviour in the game itself. Now it could be said that it's your mate's fault for using a C-compiled lodepng from C++, but it feels like there should be more guards in place to stop that from happening if that's the case. Note that without the virtual destructor the crashes/strange behaviour would go away and it'd be safe to call the C-compiled lodepng from C++ since all the types would match in size.

[lvandeve]

Ok, it's all well defined behavior, and indeed one is not supposed to subclass a State or even create one with new or delete, so worth a try.

[bobsayshilol]

Awesome, thanks!