Don't fail in gz_load() if read would block #993

drbitboy · 2024-07-18T14:49:54Z

I.e. when [errno] is EAGAIN or EWOULDBLOCK)

E.g. if file descriptor is a non-blocking TCP socket,
and more data are expected. This might cause a
problem if compressed bytes are read but do not
generate an uncompressed byte, because the return
value will be 0, which usually indicates EOF.

See madler/zlib Issue #992

I.e. when [errno] is EAGAIN or EWOULDBLOCK) E.g. if file descriptor is a non-blocking TCP socket, and more data are expected. This might cause a problem if compressed bytes are read but do not generate an uncompressed byte, because the return value will be 0, which usually indicates EOF.

drbitboy · 2024-07-18T14:50:42Z

N.B. do not merge this PR yet; I still have some work to do to see if what I am trying to do is even feasible.

…til data are transferred to buffer argument

drbitboy · 2024-07-22T13:21:35Z

gzread.c

@@ -24,7 +24,7 @@ local int gz_load(gz_statep state, unsigned char *buf, unsigned len,
            break;
        *have += (unsigned)ret;
    } while (*have < len);
-    if (ret < 0) {
+    if (ret < 0 && errno != EAGAIN && errno != EWOULDBLOCK) {


This is the core of this PR, because all of the read-related wrappers here in gzread.c
end up calling this gz_load routine: when

BOTH state-fd (file descriptor) is non-blocking (fcntl(fd, SET_FL, fcntl(fd, GET_FL) | O_NONBLOCK))

AND there are no data available to read,

then the read(2) return value (ret above, assigned on line 22) will be -1, and errno will be either EWOULDBLOCK or EAGAIN (Resource not available).

The change here prevents that case from returning -1 from gz_load() here, which in turn
allows the calling routine* to continue working with data from the file descriptor.

It gets a little tricky, because no bytes are returned, and 0 as the return value from read(2)
indicates EOF (End-Of-File).

N.B. tt may make sense to clear errno to a value of 0 here in gz_load(), just before line 22 above.

* gz_avail() or gz_fetch() or gz_read(), with the latter being the ultimate caller of the first two.

…atus of client and server suitable for automated testing (make test...)

drbitboy

This Pull Request is ready to merge, pending review and approval of the repo owner (Dr. Adler).

drbitboy · 2024-07-22T13:32:43Z

gzread.c

@@ -307,25 +309,31 @@ local z_size_t gz_read(gz_statep state, voidp buf, z_size_t len) {
           buffer */
        else if (state->how == LOOK || n < (state->size << 1)) {
            /* get more output, looking for header if required */
+            errno = 0;


Prepare for errno to become EAGAIN/EWOULDBLOCK, indicating that the read(2) on the
non-blocking file descriptor would block, so the read(2) returns a value of -1.

drbitboy · 2024-07-22T13:32:50Z

gzread.c

@@ -268,6 +268,7 @@ local int gz_skip(gz_statep state, z_off64_t len) {
 local z_size_t gz_read(gz_statep state, voidp buf, z_size_t len) {
    z_size_t got;
    unsigned n;
+    int load_errno;


Variable to keep track of errno from the read(2) in gz_load(), whether gz_load() is called from here (gz_read())

EITHER directly,

OR indirectly via gz_decomp() or gz_fetch().

drbitboy · 2024-07-22T13:36:10Z

gzread.c

            if (gz_fetch(state) == -1)
                return 0;
+            if (!state->x.have) { load_errno = errno; }


The call to gz_fetch() (line 313may have resulted in a call to gz_load()/read(2) and updated the internal output buffer, so errno may have a value of EAGAIN/EWOULDBLOCK, indicating that read(2) would have blocked on a non-blocking file.

The next line, 316, will continue to the while at the end of this do-while loop.

The state->x internal output buffer is encapsulated in state->x.next and state->x.have (see lines 293-300 above).

So if there is any data in the internal output buffer (state->x.have is non-zero), then don't copy the value, possibly EAGAIN/EWOULDBLOCK, of errno from gz_fetch() here, because that would cause the while to exit the loop and the data in the internal input buffer would be lost.

If the the state->x internal output buffer is empty here (state->x.have is 0), then there was no data read in the gz_fetch() call (line 313), and it is save to copy the errno value to load_errno and continue, which, if errno is EAGAIN/EWOULDBLOCK here, will exit this do-while loop .

drbitboy · 2024-07-22T15:56:50Z

gzread.c

            if (gz_load(state, (unsigned char *)buf, n, &n) == -1)
                return 0;
+            load_errno = errno;


gz_load() (line 324) read the data directly into the external output buffer (buf), and state->x was empty and not used here, so put the value of errno into load_errno, which if that value is EAGAIN/EWOULDBLOCK, will cause the do-while loop to exit (line 346).

drbitboy · 2024-07-22T15:58:32Z

gzread.c

            if (gz_decomp(state) == -1)
                return 0;
+            load_errno = errno;


gz_decomp() (line 334) read the data directly into the external output buffer (buf; lines 331-332), and that buffer will be declared empty when state->x.have is cleared to 0 (line 338), so put the value of errno into load_errno, which if the value is ...

drbitboy · 2024-07-24T13:16:56Z

Makefile.in

 check: test

 test: all teststatic testshared

-teststatic: static
+teststatic: static testgznonblk


Addubg the gznoblk test target as a dependency to this static test target, so the non-blocking functionality added to gzread.c in this PR can be tested.

drbitboy · 2024-07-24T13:33:34Z

Makefile.in

+	else \
+	  echo '		*** gznonblk test FAILED ***'; false; \
+	fi
+


Here are the details of the gznonblk test target, which tests the non-blocking functionality added to gzread.c in this PR. The command is

./gznonblk 44444 --client-fork 127.0.0.1 first second --delay third --delay -stopserver-

./gznonblk - run the executable built by staticgznb

N.B. both server and client sides are contained in this one executable.

44444 - the port number for the server socket

--client-fork - the server parent spawns the client child

N.B. the rest of the arguments are strings or directives for the client only

127.0.0.1 - the server host string to which the client will connect

i.e. localhost, but localhost may point to :: (IPV6) on some systems

first, second - the first two strings that the client will gzwrite to, and the server will gzread from, the socket

N.B. these two arguments are typically concatenated into a single gzread on the server side

--delay - a directive to the client to delay before processing the next argument

third - the third string gz-transferred from client to server

--delay - another delay to ensure the next and final string is not concatenated to the previous "third"

-stopserver - the final string gz-transferred from client to server

which will trigger the server to exit (see variable stopserver in test/gznonblk.c)

Once the server (parent) exits, it will pause, then check for the exit status of the client (child), and set its own return status to success (zero) IFF both server and client exited successfully.

drbitboy · 2024-07-24T13:34:34Z

Makefile.in

@@ -145,6 +154,9 @@ example.o: $(SRCDIR)test/example.c $(SRCDIR)zlib.h zconf.h
 minigzip.o: $(SRCDIR)test/minigzip.c $(SRCDIR)zlib.h zconf.h
 	$(CC) $(CFLAGS) $(ZINCOUT) -c -o $@ $(SRCDIR)test/minigzip.c

+gznonblk.o: $(SRCDIR)test/gznonblk.c $(SRCDIR)zlib.h zconf.h
+	$(CC) $(CFLAGS) $(ZINCOUT) -c -o $@ $(SRCDIR)test/gznonblk.c


Compiling test/gznonblk.c, patterned after the other test programs

drbitboy · 2024-07-24T13:35:27Z

Makefile.in

@@ -287,6 +299,9 @@ example$(EXE): example.o $(STATICLIB)
 minigzip$(EXE): minigzip.o $(STATICLIB)
 	$(CC) $(CFLAGS) $(LDFLAGS) -o $@ minigzip.o $(TEST_LIBS)

+gznonblk$(EXE): gznonblk.o $(STATICLIB)
+	$(CC) $(CFLAGS) $(LDFLAGS) -o $@ gznonblk.o $(TEST_LIBS)


Builds the gznonblk test program using the static library (libz.a)

drbitboy · 2024-07-24T13:37:56Z

test/gznonblk.c

@@ -0,0 +1,563 @@
+#include <zlib.h>


New file, the source code for testing the new gz non-blocking functionality added to gzread.c in this PR. Please feel free to let me know if the comments could be better.

drbitboy · 2024-07-25T13:32:24Z

I added a few more bells and whistles.

But note that the primary changes in this PR are the ten new lines of code in gzread.c, and each of those changes has github comments associated with it. Also, those changes should not affect applications using zlib, because errno=EWOULDBLOCK/EAGAIN will not occur for blocking reads, and anyone using non-blocking reads would not be using zlib v1.3.1's gzread in the first place because it doesn't work in that environment.

Let me know if you have any questions.

drbitboy · 2024-07-25T14:27:28Z

P.S. My style is wrong in test/gznonblk.c; I can fix that.

Neustradamus · 2024-08-22T12:57:02Z

@madler: Have you seen this PR?

drbitboy mentioned this pull request Jul 18, 2024

Non-blocking handling in gzread #992

Open

Brian T. Carcich added 3 commits July 20, 2024 15:54

Handle EWOULDBLOCK/EAGAIN in gz_load() call from gz_read

f16f656

More handling of EWOULDBLOCK/EAGAIN results in calls from gz_read

adb2b5b

In gz_read(), do not introduce blocked read state after gz_fetch() un…

6e8ddfd

…til data are transferred to buffer argument

drbitboy commented Jul 22, 2024

View reviewed changes

Brian T. Carcich added 12 commits July 23, 2024 14:17

Add non-blocking socket test code; first draft, no formal test yet

3ddf538

Rename test program

12eca61

Build GZ-non-blocking binary

1cf7557

Add fork of client by server in test/gznonblk.c; not done yet

d781605

Remove [int s]; use '-stopserver-' to stop server

73b57d4

Fix post-fork(2) bug; clean up comments

a9f3755

test/gznonblk.c now working, server spawns client and reports exit st…

ea6e677

…atus of client and server suitable for automated testing (make test...)

Update Makefile.in to run test of gznonblk

f6a01bb

Add /gznonblk to .gitignore

880c261

Add dependencies for gznonblk* to Makefile.in

6366659

Move testgznonblk dependency to the right place in Makefile.in

5c75fcb

test/gznonblk.c: add more comments, rearrange logging

5ab0725

drbitboy commented Jul 24, 2024

View reviewed changes

Brian T. Carcich added 4 commits July 24, 2024 11:34

Fix typo

511989d

Add usage options (--help[-long]) to test/gznonblk.c

970385d

Clean up test/gznonblk.c Usage

903c3ab

test/gznonblk.c: add header comments

c6152bb

Update ChangeLog

df4df93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't fail in gz_load() if read would block #993

Don't fail in gz_load() if read would block #993

drbitboy commented Jul 18, 2024

drbitboy commented Jul 18, 2024

drbitboy Jul 22, 2024 •

edited

Loading

drbitboy left a comment

drbitboy Jul 22, 2024

drbitboy Jul 22, 2024

drbitboy Jul 22, 2024

drbitboy Jul 22, 2024

drbitboy Jul 22, 2024

drbitboy Jul 24, 2024

drbitboy Jul 24, 2024

drbitboy Jul 24, 2024

drbitboy Jul 24, 2024

drbitboy Jul 24, 2024 •

edited

Loading

drbitboy commented Jul 25, 2024

drbitboy commented Jul 25, 2024

Neustradamus commented Aug 22, 2024

Don't fail in gz_load() if read would block #993

Are you sure you want to change the base?

Don't fail in gz_load() if read would block #993

Conversation

drbitboy commented Jul 18, 2024

drbitboy commented Jul 18, 2024

drbitboy Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

drbitboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drbitboy Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

drbitboy commented Jul 25, 2024

drbitboy commented Jul 25, 2024

Neustradamus commented Aug 22, 2024

drbitboy Jul 22, 2024 •

edited

Loading

drbitboy Jul 24, 2024 •

edited

Loading