Page MenuHome GnuPG

Fix FD2INT for 64-bit Windows
Open, NormalPublic

Description

FD2INT implementation as of today (2023-07-18) has potential issue.

  • It was written for 32-bit Windows and POSIX
  • For 32-bit Windows, casting with (unsigned int) works (it means, no conversion of the value)
  • For 64-bit Windows, casting with (unsigned int) means conversion of lower 32-bit.
    • This works mostly, for the case of 32-bit value of HANDLE
    • When HANDLE is within the range of 32-bit, it's OK
    • But, it may fail to represent INVALID FD (0xffffffffffffffff), the converted value is 0xffffffff

Event Timeline

gniibe triaged this task as Normal priority.Jul 18 2023, 7:15 AM
gniibe created this task.
gniibe created this object with edit policy "Contributor (Project)".

Here is a test program for 64-bit Windows to see how cast works:

#include <stdint.h>
#include <stdio.h>

int
main (int argc, const char *argv[])
{
  const char *p;
  uint64_t n0, n1;
  int n;

  p = (void *)-1;
  /* warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] */
  n0 = (unsigned int)p;         /* Not good.  It's masked.  */
  printf ("0: %p : %016llx\n", p, n0);
  n1 = (uintptr_t)p;            /* Good.  It's kept as is.  */
  printf ("1: %p : %016llx\n", p, n1);

  n = -1;

  /* works by sign extend, but warning for different size. */
  /* warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] */
  p = (void *)n;
  printf ("ok:   %p\n", p);

  /* works by sign extend, explicitly request sign extend. */
  p = (void *)(intptr_t)n;
  printf ("good: %p\n", p);

  /* works by no sign extend.  */
  p = (void *)(uintptr_t)n;
  printf ("ok:   %p\n", p);

  /* doesn't works by no sign extend.  */
  p = (void *)(uintptr_t)(unsigned int)n;
  printf ("bad:  %p\n", p);

  return 0;
}

The result is:

$ ./a.exe 
0: ffffffffffffffff : 00000000ffffffff
1: ffffffffffffffff : ffffffffffffffff
ok:   ffffffffffffffff
good: ffffffffffffffff
ok:   ffffffffffffffff
bad:  00000000ffffffff

We need to fix the macro FD2INT for 64-bit Windows.

Fixed FD2INT in: rG521ec40aea89: common,w32: Fix FD2INT macro.

Use of FD2INT for the first argument of select is semantically not good. It's the number of file descriptor. When we use FD2INT here, the type is converted to 64-bit integer, then implicitly demoted to 32-bit integer. We need new macro, say, FD2NUM to convert FD into 32-bit integer.
<--- done in: rGea1935252e28: commond: Introduce FD2NUM to express conversion to number of fds.

gniibe changed the task status from Open to Testing.Jul 19 2023, 4:53 AM

On 64-bit Windows, the situation now is:

  • FD2INT
    • Fixed for 64-bit. No masking to lower-bit any more.
  • FD2NUM
    • It's conversion to 32-bit signed integer. It is used for select call. In the select implementation, the value is not used. So, any conversion works.
    • When/if POSIX compliant select will be introduced in future, we will be able to fix the FD2NUM macro, together with the select calls.
  • FD_DBG
    • It's conversion to 32-bit signed integer. It is used to display debug message using FD. Converted value may not be accurate.
    • When/if OS will be changed in future and we will want to display accurately, we will be able to fix this FD_DBG macro, together with formatting string.

That is, everything works well now. All places with the cast have been examined and fixed when there were problems.

werner moved this task from Backlog to Done on the gnupg26 board.

I claim this resolved given that we had several gpg4win-5 betas and no reported problems was related to this.

Regarding 64bit handles https://learn.microsoft.com/en-us/windows/win32/winprog64/interprocess-communication
tells us:

64-bit versions of Windows use 32-bit handles for interoperability. When sharing a handle between 32-bit and 64-bit applications, only the lower 32 bits are significant, so it is safe to truncate the handle (when passing it from 64-bit to 32-bit) or sign-extend the handle (when passing it from 32-bit to 64-bit). Handles that can be shared include handles to user objects such as windows (HWND), handles to GDI objects such as pens and brushes (HBRUSH and HPEN), and handles to named objects such as mutexes, semaphores, and file handles.

Thus @gniibe's initial claim "When HANDLE is within the range of 32-bit, it's OK" holds true. I think it should be possible to have just one macro - or if needed use a test macro for INVALID_FD. Deciding when to use FD2INT and FD2NUM is not easy to see.

Can't you just use file descriptors everywhere and use _get_osfhandle once you need a HANDLE. That is what I am used to seeing in Windows code in Gnulib (although I do not touch it much).

Here is the documentation: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170