Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gzseek broken on uncompressed streams on Windows #95

Closed
carlobaldassi opened this issue Apr 2, 2015 · 20 comments
Closed

gzseek broken on uncompressed streams on Windows #95

carlobaldassi opened this issue Apr 2, 2015 · 20 comments

Comments

@carlobaldassi
Copy link

On Windows, when calling gzopen on a plain text file, reading some bytes (at least 1), then calling gzseek(f, 0L, SEEK_SET), all subsequent gzread calls fail (they return 0).

This does not happen when opening a compressed stream.

In contrast, calling gzrewind(f) (which should be equivalent to the above, according to the documentation in "zlib.h") always works. Also, on Linux and MacOSX there is no such issue.

I'm using zlib 1.2.8, and Windows 7 32bit (but I believe 64bit is also affected).

Here is a simple test file which shows the problem (assuming "tst.tmp.txt" is any plain text file):

#include <stdio.h>
#include <zlib.h>

int main(int argc,char *argv[])
{
    char buf[1000];
    int ret;
    gzFile f = gzopen("tst.tmp.txt", "rb");
    //gzFile f = gzopen("tst.tmp.gz", "rb"); // this works

    while (!gzeof(f)) {
        ret = gzread(f, buf, 1);
        if (!ret) {
            printf("READ FAILED\n");
        } else {
            printf("%c %i\n", buf[0], buf[0]);
        }
    }
    printf("EOF\n");
    ret = gzseek(f, 0, SEEK_SET);
    printf("SEEK RETURNED: %i\n", ret);
    while (!gzeof(f)) {
        ret = gzread(f, buf, 1);
        if (!ret) {
            printf("READ FAILED\n");
        } else {
            printf("%c %i\n", buf[0], buf[0]);
        }
    }
    printf("EOF\n");

    return 0;
}

when running the code, the first while loop succeeds, then gzseek returns 0, and subsequent gzreads fail. If the first while loop is omitted, the rest succeeds, but as long as a single byte is read the bug is observed.

carlobaldassi added a commit to JuliaIO/GZip.jl that referenced this issue Apr 9, 2015
@carlobaldassi
Copy link
Author

Additional info:

  1. the bug occurs even when seeking to a position other than 0
  2. if one calls gzrewind first, then gzseek succeeds.

carlobaldassi added a commit to JuliaIO/GZip.jl that referenced this issue Apr 9, 2015
@madler
Copy link
Owner

madler commented Apr 11, 2015

Thank you for the report. I don't have a Windows system to test this on. Perhaps you can help.

The difference in this case is that gzseek() uses effectively _lseeki64(fd, -pos, SEEK_CUR) (where pos is the current position), whereas gzrewind() uses _lseeki64(fd, 0, SEEK_SET). Do you know why those might behave differently on Windows?

@carlobaldassi
Copy link
Author

Sorry, I have no Windows experience whatsoever. I was made aware that there was a problem with a package I wrote and tracked it down, but that's as far as I can go on my own.

I can perform specific tests if given directions though, I now have a virtual machine with Windows and the mingw compiler installed.

@pzychotic
Copy link
Contributor

I tried to reproduce that error on Win 8.1 x64 with VS 2008, 2010, 2012 and 2013 in both x86 and x64 builds, but it always works correctly.

Could this be a problem with the MinGW C runtime?

@madler
Copy link
Owner

madler commented Apr 11, 2015

Carlo: Please let us know exactly on what compiler and in what run-time environment and operating system you see this problem. Thanks.

@carlobaldassi
Copy link
Author

The first report I had was from using the Julia GZip.jl package. This uses the precompiled dll which comes with zlib and calls the functions via dynamically loading the library and using the C ABI (reference). I'm not sure which version of Windows this was observed on (either 7 or 8), but I can investigate.

Then, I was able to reproduce the problem in a Windows 7 32bit virtual machine. I observed the problem described above, then I tried compiling my own executable with mingw's gcc (version 4.8.1), this time using the zdll.lib static library, and the results are those reported above. I did not use any compilation flag.

The mingw version is the latest available (0.6.2 perhaps?); I downloaded it a few days ago.

I also verified that the "fix" of calling gzrewind before gzseek also works for the case of calling the dynamically linked version from Julia (see this patch).

In my tests I made sure I was using the latest binaries downloaded from the zlib main site.

I think the Julia executable is also cross-compiled with mingw, but I'm not sure, and I don't know if that's relevant.

In case you want to try reproducing the original bug, the most straightforward way would be downloading Julia, then launching it and running:

julia> Pkg.add("FastaIO")
julia> Pkg.pin("GZip", v"0.2.14") # ensures that the patch is not included
julia> Pkg.test("FastaIO")

The last command will fail with an "empty file" error.

Let me know if there's any other detail I can provide.

@pzychotic
Copy link
Contributor

I was building zlib from source (v1.2.8 release from GitHub) with CMake and all the VS versions mentioned earlier, both static and dynamic libraries.

I just gave the pre-compiled binaries from the zlib website a quick try in a VS2013 x86 build and now I also see that problem.

The README for that binaries state that MinGW-GCC was used to build them:
Compiler:
i686-w64-mingw32-gcc (GCC) 4.5.3
Library:
mingw64-i686-runtime/headers: 3.0b_svn5747-1

Since you also see the problem with your own MinGW-GCC built version, it might suggest that the problem is somewhere in the MinGW environment. Just a wild guess, though. I never used MinGW myself and have absolutely no clue about it.

@sverx
Copy link

sverx commented Jan 5, 2016

I was fighting with the same (or a very similar) problem since yesterday. I just wanted to mention that I tried the patch detailed here and it did the trick.
For your convenience:

--- gzlib.c    2013-03-24 23:47:59.000000000 -0600
+++ gzlib.patch.c    2014-04-27 15:34:38.496808069 -0600
@@ -393,7 +393,7 @@
     /* if within raw area while reading, just go there */
     if (state->mode == GZ_READ && state->how == COPY &&
             state->x.pos + offset >= 0) {
-        ret = LSEEK(state->fd, offset - state->x.have, SEEK_CUR);
+        ret = LSEEK(state->fd, offset - (z_off64_t)state->x.have, SEEK_CUR);
         if (ret == -1)
             return -1;
         state->x.have = 0;

@madler
Copy link
Owner

madler commented Jan 30, 2016

Please let me know if this commit fixes the problem. Thanks.

@sverx
Copy link

sverx commented Feb 1, 2016

Yes, using that revision my program works as expected :)
BTW I don't get why _lseeki64 should be buggy on MinGW, as I could make that work anyway, using the small patch posted above...
Thanks!

@ismail
Copy link

ismail commented Feb 13, 2016

Are you guys using outdated MinGW packages from mingw.org or the current mingw packages from mingw-w64.org ?

@ismail
Copy link

ismail commented Feb 15, 2016

Never mind, I can reproduce the problem with mingw-w64 too.

@madler
Copy link
Owner

madler commented Feb 15, 2016

Can you copy here the prototype of _lseeki64 from the header file?

@ismail
Copy link

ismail commented Feb 15, 2016

__MINGW_EXTENSION __int64 __cdecl _lseeki64(int _FileHandle,__int64 _Offset,int _Origin);

@ismail
Copy link

ismail commented Feb 17, 2016

gzFile_s is defined as:

struct gzFile_s {
    unsigned have;
    unsigned char *next;
    z_off64_t pos;
};

so casting have to z_off64_t as in https://lists.fedoraproject.org/pipermail/mingw/2014-April/008056.html do make sense to me.

@madler
Copy link
Owner

madler commented Dec 30, 2016

Not using _lseeki64() in MinGW seems to fix the problem, so I am closing this issue. If this fix is reported to not work, then I will reopen the issue.

@madler madler closed this as completed Dec 30, 2016
@vszakats
Copy link

vszakats commented Jan 5, 2017

The merged patch effectively disables 64-bit seeks when using mingw (making it use lseek() instead), which is probably not what was intended.

Reproducible using mingw32-make -f win32\makefile.gcc.

@vszakats
Copy link

vszakats commented Jan 5, 2017

lseek64() can be forced with mingw by using these custom C flags: -D_LARGEFILE64_SOURCE=1 -D_LFS64_LARGEFILE=1. Haven't tested if it works as expected though.

@jay
Copy link

jay commented Jan 29, 2017

@madler I think Viktor makes a good point, is this what you intended?

recap
ret = LSEEK(state->fd, offset - state->x.have, SEEK_CUR);
offset is z_off64_t
state->x.have is unsigned int

On Windows when mingw gcc is used z_off64_t is z_off_t is long and so is 32-bit signed. (Why?)

So we are going to end up with is an unsigned result.

C89 standard from 3.2.1.5 Usual arithmetic conversions

Otherwise, if one operand has type long int and the other has type
unsigned int, if a long int can represent all values of an unsigned
int, the operand of type unsigned int is converted to long int ; if a
long int cannot represent all the values of an unsigned int, both
operands are converted to unsigned long int.

C99 standard from 6.3.1.8 Usual arithmetic conversions

Otherwise, if the type of the operand with signed integer type can represent
all of the values of the type of the operand with unsigned integer type, then
the operand with unsigned integer type is converted to the type of the
operand with signed integer type.
Otherwise, both operands are converted to the unsigned integer type
corresponding to the type of the operand with signed integer type.

So it seems the result would be of type unsigned long int.

_lseeki64 second param is type __int64 so this:
(__int64)(offset - state->x.have)
which fails because the unsigned long int value can always fit in __int64 on Windows so it's never converted back to negative

lseek second param is type long so this:
(long)(offset - state->x.have)
Though the conversion to signed is defined in gcc I have to wonder why you would do it that way, and not as noted earlier in this thread offset - (z_off64_t)state->x.have.

Also it's confusing to have a type named z_off64_t that can be something less than 64 bits. If it's possible to have a have value larger than Z_OFF64_T_MAX or whatever then I assume it should be checked for overflow as well.

jay added a commit to jay/zlib that referenced this issue Jan 30, 2017
- Define z_off64_t to __int64 instead of z_off_t (long)

- Define LSEEK to _lseeki64 instead of lseek

- Fix arithmetic in lseek call by casting unsigned int to __int64

madler#95 (comment)
vszakats added a commit to vszakats/hb that referenced this issue Feb 3, 2017
  * src/3rd/zlib/Makefile
  * src/3rd/zlib/zlib.diff
    % NO_VIZ macro not used by recent zlib code, stop defining it
    * define zlib macros to enable lseek64 on mingw, instead of patching
      zlib source code. Required since 1.2.9.
      Ref: madler/zlib#95 (comment)
@madler
Copy link
Owner

madler commented Feb 5, 2017

Yes, that is a problem if the type of the second argument of lseek is larger than the off_t type. However the raison d'être for off_t is to be the second argument of lseek.

ningfei added a commit to ningfei/leaddbs that referenced this issue Jun 8, 2019
gzseek problem with MinGW. See:madler/zlib#95
The fix is in znzlib.c: calling gzrewind before gzseek.
hzhuang1 pushed a commit to Linaro/warpdrive-zlib that referenced this issue Jul 31, 2019
…er#95)

* Inflate using wider loads and stores.

In inflate_fast() the output pointer always has plenty of room to write.  This
means that so long as the target is capable, wide un-aligned loads and stores
can be used to transfer several bytes at once.

When the reference distance is too short simply unroll the data a little to
increase the distance.

Change-Id: I59854eb25d2b1e43561c8a2afaf9175bf10cf674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants