Thread: Incorrect fd handling in syslogger.c for Win64 under EXEC_BACKEND

Hi all,

While reviewing a patch that refactors syslogger.c, we use the
following code to pass down a HANDLE to a forked syslogger as of
syslogger_forkexec():
    if (syslogFile != NULL)
            snprintf(filenobuf, sizeof(filenobuf), "%ld",
                     (long) _get_osfhandle(_fileno(syslogFile)));

Then, in the kicked syslogger, the parsing is done as follows in
syslogger_parseArgs() for WIN32, with a simple atoi():
    fd = atoi(*argv++);

_get_osfhandle() returns intptr_t whose size is system-dependent, as
it would be 32b for Win32 and 64b for Win64:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle
https://docs.microsoft.com/en-us/cpp/c-runtime-library/standard-types

As long is 4 bytes on Windows, we would run into overflows here if the
handle is out of the normal 32b range.  So the logic as coded is fine
for Win32, but it could be wrong under Win64.

Am I missing something obvious?  One thing that we could do here is
to do the parsing with pg_lltoa() while printing the argument with
INT64_FORMAT, no?

Thoughts?
--
Michael

On Tue, Sep 28, 2021 at 12:41:40PM +0900, Michael Paquier wrote:
> Am I missing something obvious?  One thing that we could do here is
> to do the parsing with pg_lltoa() while printing the argument with
> INT64_FORMAT, no?

I wrote that a bit too quickly.  After looking at it, what we could
use to parse the handle pointer is scanint8() instead, even if that's
a bit ugly.  I also found the code a bit confused regarding "fd", that
could be manipulated as an int or intptr_t, so something like the
attached should improve the situation.

Opinions welcome.
--
Michael

On Tue, Sep 28, 2021 at 02:36:52PM +0900, Michael Paquier wrote:
> I wrote that a bit too quickly.  After looking at it, what we could
> use to parse the handle pointer is scanint8() instead, even if that's
> a bit ugly.  I also found the code a bit confused regarding "fd", that
> could be manipulated as an int or intptr_t, so something like the
> attached should improve the situation.

As reminded by Jacob, the code is corrently correct as handles are
4 bytes on both Win32 and Win64:
https://docs.microsoft.com/en-us/windows/win32/winauto/32-bit-and-64-bit-interoperability

Sorry for the noise.  It looks like I got confused by intptr_t :p
--
Michael