Skip to content

Add the missing loop exit condition based on Z_STREAM_END #286

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harukat
Copy link

@harukat harukat commented May 29, 2025

doDeflate() function has no loop exit condition if deflate() returns Z_STREAM_END.
Therefore, this code could potentially cause an infinite loop.
Similar code using zlib in PostgreSQL has such an exit condition, so it would be desirable to do the same for pg_rman.

…) return.

doDeflate() function has no loop exit condition if deflate() returns
Z_STREAM_END. Therefore, this code could potentially cause an infinite loop.
Similar code using zlib in PostgreSQL has such an exit condition,
so it would be desirable to do the same for pg_rman.
@shinyaaa shinyaaa self-assigned this Jun 3, 2025
@shinyaaa shinyaaa self-requested a review June 3, 2025 11:37
@atorik
Copy link
Contributor

atorik commented Jun 5, 2025

I don't have prior experience working directly with zlib, so I may be misunderstanding something.
If there is anything incorrect in my understanding, please feel free to point it out.

Based on my reading of zlib.h, it seems that deflate() returns Z_STREAM_END only when flush is set to Z_FINISH and all input has been fully processed:

 -- zlib-1.2.11/zlib.h
 326   If the parameter flush is set to Z_FINISH, pending input is processed,
 327   pending output is flushed and deflate returns with Z_STREAM_END if there was
 328   enough output space.

This suggests that a situation where zp->avail_in is non-zero while deflate() returns Z_STREAM_END should never occur.
If it does, it may indicate a bug in the zlib library itself.

If we want to explicitly handle this case, I think it would be more appropriate to add an assertion (e.g., Assert( status == Z_STREAM_END) when exiting the loop, following the example style from the official zlib site, rather than introducing a new condition to break the loop.

Here is the relevevant snippet from the site:

     /* compress until end of file */
     do {
     ...
         flush = feof(source) ? Z_FINISH : Z_NO_FLUSH;
     ...
         /* run deflate() on input until output buffer not full, finish
            compression if all of source has been read in */
         do {
     ...
             ret = deflate(&strm, flush);    /* no bad return value */
             }
         } while (strm.avail_out == 0);
         assert(strm.avail_in == 0);     /* all input will be used */

         /* done when last data in file processed */
     } while (flush != Z_FINISH);
     assert(ret == Z_STREAM_END);        /* stream will be complete *

@harukat
Copy link
Author

harukat commented Jun 6, 2025

I understand that this fix is not mandatory.
My proposed fix just mimics the following PostgreSQL codes.

  • bin/pg_dump/compress_gzip.c: DeflateCompressorCommon()
  • bin/pg_basebackup/walmethods.c: tar_write_compressed_data()

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

Successfully merging this pull request may close these issues.

3 participants