Avoid mixing incompatible CRTs when linking to libpng.dll

[pps83]

If libpng.dll is built using static linking (without importing C runtimes from whatever version of ms compiler that was used to build it), or when it was build by mingw then such dll won't work properly as stdio fread/fwrite will try to interpret FILE pointers from unrelated CRT and will result in undefined behavior.

[pps83]

In my case I built a libpng dll using MS VS2015 compiler and wanted to run tests to validate that everything is ok. To run tests I compiled a separate version of libpng using msys/mingw and simply replaced dll with my own build. Some of tests failed:

I tried to debug and it appears that FILE pointers from the side of test executable are passed across DLL boundary. This change introduces three new api functions: png_init_io2, png_init_read_io and png_init_write_io and these differ from png_init_io by passing function pointers to fread/fwrite/fflush along with FILE pointer.
Also, png_init_io is redefined as a macro that forwards FILE pointer to png_init_io2 along with fread/fwrite/fflush. This way simple recompilation of existing code that uses png_init_io will make it use png_init_io2.

No changes were needed to code that uses libpng and all the tests passed. In short, libpng could use stdio internally and create/read/write/close files but if these pass across api boundary then it might become an issue with static builds on windows.

[jbowler]

This is an attempt to fix an unfixable problem. The only fix is, in fact, a one liner; delete png_init_io from the API.

If an app creates a FILE* to use by libpng it must either use the "set...fn" APIs and set the read/write/flush functions (exactly as if it were any other IO device pointer) or it must call png_init_io and rely on libpng to call the stdio fread/fwrite (etc) APIs.

On Microsoft systems a DLL and an application which uses it may link to equivalent but different implementations of the same API, however this means that on those systems data that is manipulated by the API may not safely be passed between the application and DLL. This is a well known fact.

png_init_io violates this requirement and can't be changed to not violate it. The alternative png_read/write_fn APIs are ok (since the io_ptr is not interpreted by libpng if the API is called correctly).

So the only fix is to delete png_init_io from the API, however on non-Microsoft systems and even on MS systems with static linking the API is incredibly useful; it makes writing PNG read/write code incredibly easy.

The "simplified" API fixes this but the cost is large; the API does not permit an arbitrary ANSI-C compliant IO pointer to be used.

So this fix is an unbelievably complex way of not fixing anything: the real fix to the reported issue is to delete line 1597 of png.h (PNG_EXPORT(74, void, png_init_io...) and that is a one line fix.

[pps83]

This is an attempt to fix an unfixable problem. The only fix is, in fact, a one liner; delete png_init_io from the API.

Yes, effectively, that's not ok to use FILE pointer where it wasn't created. My PR still keeps png_init_io as exported API for binary compatibility, but makes all new code use png_init_io2 instead which will make sure that fread/fwrite/fflush are used from the side of the user of libpng.

Deleting png_init_io from the API could break some software where it was a non-issue (*nix world basically).

[jbowler]

Yes, effectively, that's not ok to use FILE pointer where it wasn't created.

Right, but my suggestion (which doesn't really work for reasons I will explain) was to remove png_init_io with the intention of removing all the stdio.h code. This takes libpng out of the IO business; all IO happens in the app. This eliminates a bug-farm. It's not just the mixed-implementation problem on WindowsNT which, as you point out, happens anywhere (not just Windows) with a static link of a libpng DLL which includes the stdio implementation. There are other issues such as the fact that libpng does not work with a non-blocking FILE*; it can take an app programmer a long time to find that out!

It's easy to give sample code and it already exists in many places which don't have the #ifdef mess of pngrio.c and pngwio.c; app writers can just copy'n'paste this into their programs.

Indeed libpng has always supported this via PNG_NO_STDIO and (now) "option stdio off" (I'm quoting from memory; I currently don't have access to my systems.) App developers can use this to verify forward compatibility.

The gotcha is that removing stdio support from libpng does not remove the need for png_init_io, although it does remove the definition. This is shown by the simplified API code, which I intended to use just the exported APIs (so it could be extracted to a separate library). See line 1484; originally the code called png_init_io, but then it didn't build with PNG_NO_STDIO so I had to violate the libpng abstraction by directly assigning to the io_ptr.

You can see that png_init_io is not specific to stdio. It's also horribly misnamed; it does not do any initialization, it just sets io_ptr. It can also be called multiple times and called after the read/write functions are set up.

So libpng faces a choice: either remove stdio support completely, which is bound to annoy someone, or isolate it properly which is what your proposal would do. In fact your proposal could be adopted and immediately deprecated; that would preserve the existing API for one major release then drop it.

The functionality of png_init_io still has to stay but it should renamed as png_set_io_ptr. The png_init_io_2 name is just plain confusing: it's png_init_stdio and should take a FILE* argument. Having a macro which just takes a FILE* and passes read/write/flush from the C functions seems fine; this would be a lot easier with C++ :-)

The rewrite of the simplified API is a larger change of course but some of the non-core test programs use the stdio API. I'm not sure it is tested by the core tests (IRC pngstest uses memory IO).

So ABI compatibility is dropped, which is fine on a major release, but API compatibility remains with some things deprecated. I think just #define png_init_io png_set_io_ptr works because the code that initializes the read/write functions to stdio can just output a png_warning about use of a deprecated API. I don't know if your change does that already; I'm suggesting a slightly different structure here but it's not that different based on a my cursory examination.

This still isn't a complete review; I'm not discussing specifics of your code here, just exploring general issues. There are lots of opportunities for code cleanup here and it might be really good to put all the stdio stuff in a new file, 'pngstdio.c' for example, if @ctruta is amenable to adding source files to the build.

[ctruta]

Personally I wouldn't mind adding new source files. There is a problem, however, with projects that use their own custom build systems. Not a big problem, mind you, but a disruptive one, nonetheless.

OTOH libpng-ng is a good opportunity for us to break our current big and giant png*.c source files into smaller and more manageable ones.

[jbowler]

OTOH libpng-ng is a good opportunity for us to break our current big and giant png*.c source files into smaller and more manageable ones.

Certainly. 10,000 line files are just plain bad and in static builds apps which use libpng end up getting stuff that can never be called. The whole 'minconfig' stuff (scripts/pnglibconf.dfa etc) is a messy #define approach to fix that.

The directory layout also leaves much to be desired. It seems that there is a meta-standard these days to put the implementation in src/ and the interface in inc/. That strikes me as good.

[jbowler]

#622 resolves this in all three APIs which use FILE.

[jbowler]

#622 is rejected by @ctruta so this solution, which is the one @ctruta favours, needs to be elevated.