Enhancements for system libraries

[smcv]

unzip.[ch] and ioapi.[ch] are available as a system library on Debian, Ubuntu, Fedora.

While looking at the relevant section of the Makefile I noticed that I hadn't added support for libjpeg.pc because IJG libjpeg doesn't have it; but libjpeg-turbo does.

[smcv]

@ensiform in OpenJK points out that Q3's minizip has been modified to return Z_Malloc'd memory, so it isn't really compatible with the original minizip any more and is effectively a minor fork; so please disregard that part. I'll propose a replacement patch to make unzip.c mention why a system version is not suitable.

[ensiform]

I'm not sure if this is the case in ioquake3 (?)

[zturtleman]

It is the case, ioquake3's minizip uses Z_Malloc too. qcommon/unzip.c#L64

[smcv]

The replacement patch here just adds a comment to indicate to packagers that this unzip.c is not interchangeable with upstream minizip, so they don't make the same mistake I did; please consider applying that instead.

The libjpeg commit is still valid.

[lnussel]

note that the built in libjpeg also used to call z_malloc. looks like that got lost somehow. might be that libjpeg leaks memory now.

[smcv]

note that the built in libjpeg also used to call z_malloc

I don't think this is a problem, because unlike our use of minizip, the JPEG code only allocates memory internally: everything that is allocated with system malloc is freed by the time control returns to the Q3 engine. The problem with the use of minizip was that it allocates memory, returns control to the Q3 engine, and relies on the Q3 engine to free it later - which is not necessarily done when minizip's allocator is other than Z_Malloc, because the engine assumes that clearing the Z_Malloc memory pool at each ERR_DROP or level transition is sufficient.

The JPEG decompressor does this:

1. start decompression for the JPEG (allocates memory)
2. read the scanlines (raw bitmap) from the libjpeg decompression context into a ri.Malloc'd buffer (possibly allocates memory)
3. free all libjpeg resources
4. return the ri.Malloc'd buffer

The only possibility of leaking memory is if we do an early return from steps 1 or 2.

We can theoretically early-return via R_JPGErrorExit, but that's ERR_FATAL, so leaked memory is irrelevant: the process is about to die anyway. Also, it explicitly frees what it allocated.

The ri.Error(ERR_DROP, ...) if the JPEG is not 24-bit, or its width × height overflows, does not seem to be a leak because it explicitly frees what it allocated.

Similarly, the compression code-path explicitly frees libjpeg resources if it hits an error.

The minizip case could either have been solved by using the modified minizip, or by maintaining a list of closures (void * for what to free + void (*) (void *) for how to free it) for cleanup functions. I could put together a proof-of-concept patch for a cleanup list if desired, but I suspect you don't want that.

The modified minizip is certainly a lot simpler, and there's so little code in minizip that considering it to have been forked by Q3 is not such a big deal - as far as I can see, every minizip security flaw has been a path traversal in the command-line utility, rather than a bug in the parts embedded in ioquake3. (Contrast with libjpeg/libpng/zlib, which have had security flaws in the actual library code, making distributions unwilling to use embedded code copies for those.)

[timangus]

One reason to have everything use the same malloc is to ease console ports, where usually a custom malloc is required to make use of the memory correctly/efficiently. At least this was true in 1999.

[smcv]

Good point, but if someone wants to compile ioquake3 for a 1999-era console they can still use appropriately modified libraries; that doesn't have to mean that such libraries are mandatory for Windows, Mac, Linux, *BSD users (of which all except Windows already have libjpeg built into their OS).

[timangus]

Yeah, I don't particularly care, just pointing it out.