Deflate failures confirmed in downstreams using 1.2.10 / 1.2.11

[jeremyjh]

Sorry this is not a more helpful bug report as I don't know the nature of the issue or have a simple test program but I thought I should leave it here in case it helps others.

I found a problem this week in the Erlang dialyzer program on Arch Linux; it was not able to recognize its own file formats after an update. I isolated the failure to the recent zlib update and then found someone else from the Arch forum had also found a zlib problem in an SDL game.

@michelboaventura had also seen the dialyzer issue using 1.2.11 on gentoo, after hearing it was linked to zlib he was kind enough to bisect the zlib commit history and isolated the fault to b516b4b (this is reported in this thread). The same commit was confirmed to be where the issue was introduced in the manaplus game mentioned in the Arch thread.

[madler]

Isolating it to that commit points to a problem in the application code, where it must be inadvertently stomping on the deflate state, e.g. with an out-of-bounds write into memory, or perhaps that the code is trying to use the deflate state after it has been closed. The only change that commit made was to check the integrity of the deflate structure more thoroughly on each call of a deflate* function.

[michelboaventura]

Hi @madler,

If tested every change on that commit and figure out these two ifs which returned false before the commit started to return true afterwards:
b516b4b#diff-327188edf18799ffbb5a51cc69f797e8R645
b516b4b#diff-327188edf18799ffbb5a51cc69f797e8R1281

Checking what is now being checked on inflateStateCheck that wasn't I find out the issue is with the state->strm != strm bit. So they were probably not equal but this wasn't checked until this commit.

May you guide me to better debug it? What is the issue with those two 'strm' being equal, and how the erlang code could have used it in a wrong way?

[sverker]

The Erlang VM does a shallow memcpy of z_stream and commit b516b4b
introduced a back pointer from the internal inflate_state to the (old deallocated) z_stream.

Seems inflateCopy() is the solution and the correct way to do it.
This bug has existed since Erlang/OTP 17.0.

/Sverker, Erlang/OTP @ Ericsson

[madler]

Thank you for the resolution. I will close this as it is not a zlib issue.

[jeremyjh]

@madler thanks for your help!

[michelboaventura]

Thank you @sverker and @madler for the help and thanks to @jeremyjh to help to pin point the issue.

[4144]

I am developer of this SDL game.
I tracked issue to libphysfs and zlib.

Here is zip archive with example: http://download.evolonline.org/manaplus/bug/bug.zip
Unpack it, and run ./run

Probably error in physfs, and i will report same issue to physfs developer.

[4144]

It was same issue in physfs. Replacing memcpy to inflateCopy fixed issues

[sverker]

@4144 and don't forget to deallocate abandoned z_stream with inflateEnd.

[madler]

Of course, if you're just going to copy it and then abandon the original, you could instead simply transfer the original.

[4144]

I think it's ok only with replacing memcpy to inflateCopy. But i not familar with zlib api.

Here is code with bug: https://hg.icculus.org/icculus/physfs/file/34ebe997c5c0/archivers/zip.c#l330
and here fix: http://download.evolonline.org/manaplus/bug/zip_seek.diff

Physfs devs still not responding...

[sverker]

@madler I didn't find any support in the API to "transfer" a z_stream.

[sverker]

@4144 inflateCopy will give you two independent z_stream's and you need to deallocate them both, the original and the copy.

[sverker]

@madler Maybe I misunderstood you. I have a stack allocated z_stream and I need to store it in a heap allocated structure i order to return back and do other work for a while. By transfer I thought you ment an optimized "inflateMove" operation that reuses (transfer) the internal_state to a new z_stream.

[madler]

By "transfer" I mean simply provide the pointer. It sounded like someone was copying an inflate state, destroying the original, and proceeding to use the copy, when they could have just used the original instead without ever copying. I understand from your post that the state is on the stack of a routine that is returning, and so needs to be moved to a safer place. Then it makes sense to copy and immediately destroy the original. Though you could have created it on the heap in the first place.

[4144]

@sverker thanks, physfs patch updated.