fetch_disk_trailer: Don't truncate the size verif

[keentux]

• When ZZIP_DISK64_TRAILER defined we must check if the tail obtained have the size of the zzip_disk_trailer struct. end - tail should be at least >= of the size but not size - 2.
• Fix Stack-buffer-overflow in function __zzip_fetch_disk_trailer() at zzip/zip.c:340 #165

[gdraheim]

I dont quite understand this patch with its ifdef-moves. Isnt ist just about removing the "-2" on the if-clause?

[keentux]

Indeed, it's all about removing the "-2" from the if-clause.
But, in same time, it seems that if the ifndef is true, as explained in the comment bloc the value of (end - trailer) could eventually be the full size of disk_trailer struct minus 2.

                    /* if the file-comment is not present, it happens
                       that the z_comment field often isn't either *

And to avoid having multiple time if condition in the case where %ifndef isn't true, I thought moving the if condition into each section of the ifndef would be a good option. I found this way to do more optimized that only add a second loop condition if (end - tail >= __sizeof(*trailer)) after the #else

[gdraheim]

Ahhh, it is just now that I see the if-clauses have been doubled. Sorry for missing that part when reading the patch first.

However I dont like that the if-conditions are different just for being able to spot a ZIP64 trailer. The code had been setup in just the way that if ZIP64 support is off (for having a very old compiler), we can still see the section in the file and get over it.

Actually, when re-reading the code, I have the impuls to remove the ifdefs all along. Support for pre-C99 compilers should not be thing anymore - it was when I started to write the library which goes all the way back to the 90ies.

[keentux]

Thanks for these additional information, I understand the need of the ifdefs and yes I agree with you about the support for pre-C99 should not be thing anymore :)

Following that, I am going to change the PR in just removing the "-2" from the if conditions without touching the ifdef and avoid having a duplicated condition. The changes should be less complex in that way.

[gdraheim]

Some tests failed and showed that the patch is only correct for the ZIP64 trailer but for the traditionalzip trailer it is best to allow the comment field to be actually missing.

[keentux]

Oh, Thanks for the verification.

[keentux]

zziplib/zzip/zip.c

Line 306 in 9388abc

memcpy(trailer, tail, sizeof(*trailer) - sizeof_z_comment);

Shouldn't be "__sizeof_z_comment" instead of "sizeof_z_comment" ?

[gdraheim]

I dont see how that makes a difference - it is a precompiler-define anyway