Re: pg_dump/pg_restore: Fix stdin/stdout handling of custom format on Win32

On Thu, Mar 2, 2023 at 8:01 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>
> We had better make sure that this does not break again 10260c7, and
> these could not be reproduced with automated tests as they needed a
> Windows terminal. Isn't this issue like the other commit, where the
> automated testing cannot reproduce any of that because it requires a
> terminal? If not, could it be possible to add some tests to have some
> coverage? The tests of pg_dump in src/bin/pg_dump/t/ invoke the
> custom format in a few scenarios already, and these are tested in the
> buildfarm for a couple of years now, without failing, but perhaps we'd
> need a small tweak to have a reproducible test case for automation?
>

I've been able to manually reproduce the problem with:

pg_dump --format=custom > custom.dump
pg_restore --file=toc.txt --list custom.dump
pg_dump --format=custom | pg_restore --file=toc.dump --use-list=toc.txt

The error I get is:

pg_restore: error: unsupported version (0.7) in file header

I'm not really sure how to integrate this in a tap test.

> The internal implementation of _pgstat64() is used in quite a few
> places, so we'd better update this part first, IMO, and then focus on
> the pg_dump part. Anyway, it looks like you are right here: there is
> nothing for FILE_TYPE_PIPE or FILE_TYPE_CHAR in this WIN32
> implementation of fstat().
>
> I am amazed to hear that both ftello64() and fseek64() actually
> succeed if you use a pipe:
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/fseek.html
> Could it be something we should try to make more portable by ourselves
> with a wrapper for these on WIN32? That would not be the first one to
> accomodate our code with POSIX, and who knows what code could be broken
> because of that, like external extensions that use fseek64() without
> knowing it.
>

The error is reproducible in versions previous to win32stat.c, so that
might work as bug fix.

> - if (hFile == INVALID_HANDLE_VALUE || buf == NULL)
> + if (hFile == INVALID_HANDLE_VALUE || hFile == (HANDLE)-2 || buf ==
> NULL)
> What's the -2 for? Perhaps this should have a comment?
>

There's a note on _get_osfhandle() [1] about when -2 is returned, but a
comment seems appropriate.

+ fileType = GetFileType(hFile);
> + lastError = GetLastError();
> [...]
> if (fileType == FILE_TYPE_UNKNOWN && lastError != NO_ERROR) {
> + _dosmaperr(lastError);
> + return -1;
> }
> So, the patched code assumes that all the file types classified as
> FILE_TYPE_UNKNOWN when GetFileType() does not fail refer to fileno
> being either stdin, stderr or stdout. Perhaps we had better
> cross-check that fileno points to one of these three cases in the
> switch under FILE_TYPE_UNKNOWN? Could there be other cases where we
> have FILE_TYPE_UNKNOWN but GetFileType() does not fail?
>

I don't think we should set st_mode for FILE_TYPE_UNKNOWN.

> Per the documentation of GetFileType, FILE_TYPE_REMOTE is unused:
>
> https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfiletype
> Perhaps it would be safer to fail in this case?
>

+1, we don't know what that might involve.

[1]
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170

Regards,

Juan José Santamaría Flecha