free_aligned: validate passed in pointer

[mtl1979]

Especially when replacing zlib with zlib-ng in old binaries, it is possible that the binary is mixing allocation and deallocation functions from different libraries. As not all old binaries can be rebuild, we should validate the passed in pointer and if the pointer doesn't seem to be allocated with alloc_aligned of zlib-ng, we should not try to adjust the pointer.

With normal allocator, the difference between pointers should be at least sizeof(void *) (either 4 or 8), but less than 128, as we align on 64-byte boundary.
Using 127 instead of 64 should allow using mask on higher bits to see if they differ.

[KungFuJesus]

I'm half tempted to capture the function pointers at initialization time for comparison and crash and burn with a scary message if they differ but...if people have been screwing this up for over a decade I guess we roll with the punches? I really wish vanilla zlib detected this and failed to begin with, this would have otherwise been caught by downstream consumers a long time ago.

There are cases with vanilla zlib that this is likely to produce problems. If dealing with MSVCRT, for instance, and the user decides to use _mm_malloc() (because microsoft doesn't provide real aligned allocators), structures that get freed later in the struct with vanilla free would crash, similarly.

[nmoinvaz]

Some of the description in the PR should be in the commit message I think.

[KungFuJesus]

I guess at the very least the warning that prints out here will help catch with a wide net projects that are using this functionality incorrectly. We also sparingly call the allocators too, so that helps.

[KungFuJesus]

Also, if the user does manage to do this, and the allocator is not an aligning allocator, won't this break code that assumes aligned allocations? There are a few places where we assume the pointer, particularly to the window, is aligned (slide_hash is one that comes to mind). And for things like VMX on PPC, this can also be a source of issues.

[mtl1979]

Also, if the user does manage to do this, and the allocator is not an aligning allocator, won't this break code that assumes aligned allocations? There are a few places where we assume the pointer, particularly to the window, is aligned (slide_hash is one that comes to mind). And for things like VMX on PPC, this can also be a source of issues.

It would most likely crash earlier.

[nmoinvaz]

won't this break code that assumes aligned allocations

That is the purpose of the alloc_aligned function, to ask for more memory than is required and then return a 64-byte aligned pointer.

[mtl1979]

Some of the description in the PR should be in the commit message I think.

I can amend the commit message after all CI runs pass...

[KungFuJesus]

Ahhh, I guess the fact that it's a mismatched free then this doesn't become an issue. And I guess given that an allocator will always only over allocate and not under allocate, this canary should suffice for most corner cases I'm thinking of.

There is the fact that we're managing to allocate with a different allocator than the user wanted to specify, which maybe creates some unexpected consequences for them (in particular if it's a debugging allocator), but yeah that's on them, I guess.

[KungFuJesus]

Perhaps we can provide a permanent URL in the warning message that provides a lineage to the zlib manual to explain what the user is doing wrong? We've run into this before with fwupd and I similarly had to dig out the documentation to point out what the real problem was. And ultimately, I think the user might appreciate us pointing to the fact their allocator they want to use is not being used where they expect it to.

[mtl1979]

Perhaps we can provide a permanent URL in the warning message that provides a lineage to the zlib manual to explain what the user is doing wrong? We've run into this before with fwupd and I similarly had to dig out the documentation to point out what the real problem was. And ultimately, I think the user might appreciate us pointing to the fact their allocator they want to use is not being used where they expect it to.

Currently the message is only shown when trace output is enabled. I thought about calling fprintf directly so the warning is always displayed... This would make any potential regressions in zlib-ng also display the warning.

FYI. MinGW i686 failure should be fixed by #1711.

[KungFuJesus]

Perhaps we can provide a permanent URL in the warning message that provides a lineage to the zlib manual to explain what the user is doing wrong? We've run into this before with fwupd and I similarly had to dig out the documentation to point out what the real problem was. And ultimately, I think the user might appreciate us pointing to the fact their allocator they want to use is not being used where they expect it to.

Currently the message is only shown when trace output is enabled. I thought about calling fprintf directly so the warning is always displayed... This would make any potential regressions in zlib-ng also display the warning.

FYI. MinGW i686 failure should be fixed by #1711.

Eh, maybe tracev is the right move, then. I guess it'd be obnoxious to have stderr dumping over their console if a closed source binary was the culprit. It's a shame though, I feel like it should be more visible, but maybe not invasively so.

[mtl1979]

Eh, maybe tracev is the right move, then. I guess it'd be obnoxious to have stderr dumping over their console if a closed source binary was the culprit. It's a shame though, I feel like it should be more visible, but maybe not invasively so.

Once in a while, I actually value feedback from other people. The issue with closed source binaries is that we can't really force them to behave... Even if this PR skips adjusting the pointer, it's still responsibility of the binary to use a free hook that can handle the allocated memory behind the pointer. Using fprintf directly would be kind of making it as obvious as possible that the code is likely going to "derail"... Things would be a lot easier with C++ as we could just catch the exception from bad free...

[KungFuJesus]

I agree and our number of allocations in inflate and deflate are limited but I do worry it'd draw the ire of a lot of users incidentally using zlib-ng, such as those using Fedora.

[mtl1979]

I agree and our number of allocations in inflate and deflate are limited but I do worry it'd draw the ire of a lot of users incidentally using zlib-ng, such as those using Fedora.

It's delicate balance between having descriptive error messages and crashing when people do something really stupid. Because zlib doesn't crash, we should try really hard not to crash. We can't be 100% binary compatible even if we wanted.