Idea for adding simple sanity checks

[thealberto]

Hi @ctruta, I have noticed that there are few functions that can be "directly" called by a user but that are lacking some checks. An example is png_save_uint_32 ( link ).

For instance it would be possible to cause a segmentation fault:

➜  oss-fuzz git:(master) ✗ cat png_save.c
#include <stddef.h>
#include <png.h>

int main() {
	png_save_uint_32(NULL, 1);
}
➜  oss-fuzz git:(master) ✗ gcc png_save.c -o png_save -lpng
➜  oss-fuzz git:(master) ✗ ./png_save                      
[1]    133879 segmentation fault (core dumped)  ./png_save
➜  oss-fuzz git:(master) ✗ 

Would you be interested in me trying to identify this cases and patch them? Do you want a separated PR for each case or bulk PR is acceptable?

Thanks

[ctruta]

@thealberto ah, the good old billion dollar mistake https://en.wikipedia.org/wiki/Tony_Hoare

png_save_uint_32 and all of its friends must be as fast as possible. They are used all over the place in libpng, and are most likely inlined everywhere. This is the C programming language, which (if you please don't mind me saying so) is not really a high-level programming language, but rather a high-level assembly. The standard C functions are just like that, all the way to the fundamental functions, including all the memory and string functions, and all the stdio functions also.

All of these functions should require valid pointers (not NULL, not unallocated, not previously deallocated, etc.) The responsibility of passing valid pointers should be on the shoulders of the libpng user, just as it is the case with the responsibility of the C stdlib/stdio user.

libpng is inconsistent in this sense, because somebody at some point did start checking some of the png_xxx functions in order to "gracefully degrade" the functionality; and then, they configured the checks with PNG_USELESS_TESTS; and then they removed this configuration macro; etc. It's very inconsistent, semantically speaking.

I contemplate turning those "graceful degradations" into assertions, for the future (post-1.x) versions of libpng.

[thealberto]

@ctruta , I can understand your point of view. Would you park this idea then until a newer version of libpng? Maybe I could work on it in the future.

Thanks

[jbowler]

SEGV is the same as ABRT; both are perfectly good ways of terminating a program with a fatal bug and there really isn't a better way; is someone going to check a return code on something that just writes 4 bytes?

[thealberto]

I can see your point regarding who is going to check the return value but in my opinion error conditions should be generally avoided.

[jbowler]

The error condition is passing a NULL pointer; it's an error in the application program.

I'm 100% in favor of deprecating functions like this; the read ones aren't used by default either within libpng or in application programs. You will notice that Glenn, or @ctruta, turned the function in question off in oss-fuzz.