Thread: Re: No error checking when reading from file using zstd in pg_dump
16.06.2025 14:25, Daniel Gustafsson пишет: >> On 16 Jun 2025, at 10:14, Evgeniy Gorbanev <gorbanyoves@basealt.ru> wrote: >> In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always >> returns true. But if you look at the Zstd_gets and Zstd_getc functions, >> where Zstd_read is called via CFH->read_func, it is expected that >> the Zstd_read function can also return false. In case of >> a read-from-file error, the process is expected to terminate, but >> pg_dump will continue the process. >> I assume that after checking >> if (cnt == 0) >> should be >> return false; > if (cnt == 0) > - break; > + return false; > > As cnt is coming from fread() returning false here would be wrong as you cannot > from 0 alone know if it's EOF or an error. Instead it needs to inspect the > stream with feof() and ferror() to know how to proceed. > > -- > Daniel Gustafsson The feof() check is in the calling functions, e.g. in the Zstd_getc function. Regards, Evgeniy
> On 16 Jun 2025, at 10:52, Evgeniy Gorbanev <gorbanyoves@basealt.ru> wrote: > > 16.06.2025 14:25, Daniel Gustafsson пишет: >>> On 16 Jun 2025, at 10:14, Evgeniy Gorbanev <gorbanyoves@basealt.ru> wrote: >>> In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always >>> returns true. But if you look at the Zstd_gets and Zstd_getc functions, >>> where Zstd_read is called via CFH->read_func, it is expected that >>> the Zstd_read function can also return false. In case of >>> a read-from-file error, the process is expected to terminate, but >>> pg_dump will continue the process. >>> I assume that after checking >>> if (cnt == 0) >>> should be >>> return false; >> if (cnt == 0) >> - break; >> + return false; >> >> As cnt is coming from fread() returning false here would be wrong as you cannot >> from 0 alone know if it's EOF or an error. Instead it needs to inspect the >> stream with feof() and ferror() to know how to proceed. > > The feof() check is in the calling functions, e.g. in the Zstd_getc > function. That function is using feof/ferror to log an appropriate error message, what you are proposing is to consider all cases of EOF as an error. If you test that you will see lots of test starting to fail. -- Daniel Gustafsson
16.06.2025 15:43, Daniel Gustafsson пишет: >> On 16 Jun 2025, at 11:26, Evgeniy Gorbanev <gorbanyoves@basealt.ru> wrote: >> I ran tests for pg_dump and they all passed. Logs attached. > Files=7, Tests=11918, 35 wallclock secs ( 0.59 usr 0.07 sys + 7.92 cusr 4.32 csys = 12.90 CPU) > > That seems like a low number of tests executed, with ZStd enabled I see over > 13200 tests in the log. Are you sure you are building with ZStd compression > enabled? > > -- > Daniel Gustafsson > Yes, you are right. Now I see the errors.
> On 16 Jun 2025, at 15:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've not checked to see what the other users of this API do, but
> if they're all like this then we need to fix that comment.
AFAICT all other callers of this API are throwing an error with pg_fatal, and
so does the function in question for ZStd decompression errors. If we handle
the case of fread() returning 0 to indicate an error like the below *untested
sketch* (with a better error message) this function is fully API compliant as
well.
/* If we have no more input to consume, we're done */
if (cnt == 0)
+ {
+ if (ferror(unconstify(void *, input->src)))
+ pg_fatal("could not read data to decompress: %m");
+
break;
+ }
If this seems like a good approach then Zstd_getc can be simplified as well as
it no longer needs to call ferror, it still needs to check feof though.
--
Daniel Gustafsson
On 6/16/25 19:56, Tom Lane wrote: > ... > > After looking around a bit more I realized that this API is a complete > disaster: it's not only bug-prone, but there are actual bugs in > multiple callers, eg _ReadBuf() totally fails to detect early EOF as > it intends to. We need to fix it, not slap some band-aids on. > Draft patch attached. > > The write_func side is a bit of a mess too. I fixed some obvious API > violations in the attached, but I think we need to do more, because > it seems that zero thought was given to reporting errors sanely. > The callers all assume that strerror() is what to use to report an > incomplete write, but this is not appropriate in every case, for > instance we need to pay attention to gzerror() in gzip cases. > I'm inclined to think that the best answer is to move error reporting > into the write_funcs, and just make the API spec be "write or die". > I've not tackled that here though. > > I was depressed to find that Zstd_getc reads one byte into > an otherwise-uninitialized int and then returns the whole int. > Even if we're lucky enough for the variable to start off zero, > this would fail utterly on big-endian machines. The only > reason we've not noticed is that the regression tests do not > reach Zstd_getc, nor most of the other getc_func implementations. > > Now I'm afraid to look at the rest of the compress_io.h API. > If it's as badly written as these parts, there are more bugs > to be found. > Well, that doesn't sound great :-( Most of this is likely my fault, as the one who pushed e9960732a961 (and some commits that built on it). Sorry about that. I'll take a fresh look at the commits this week, but considering I missed the issues before commit ... For a moment I was worried about breaking ABI when fixing this in the backbranches, but I guess that's not an issue for tools like pg_dump. regards -- Tomas Vondra
Tomas Vondra <tomas@vondra.me> writes:
> For a moment I was worried about breaking ABI when fixing this in the
> backbranches, but I guess that's not an issue for tools like pg_dump.
Yeah, I think it'd be okay to change compress_io.h APIs in the back
branches; it's hard to see how anything outside pg_dump+pg_restore
would be depending on that.
regards, tom lane