diff mbox series

[Openvpn-devel] Improve format specifier for socket handle in Windows

Message ID 20230210133159.1336-1-lstipakov@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Improve format specifier for socket handle in Windows |

Commit Message

Lev Stipakov Feb. 10, 2023, 1:31 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

Socket is a handle on Windows, which is usually logged in hex.
Also an interesting value is INVALID_SOCKET, which is ~0.

PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like

  2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle
annettu data-alue on liian pieni.   (fd=18446744073709551615,code=122)

PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer:

  2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle
annettu data-alue on liian pieni.   (fd=ffffffffffffffff,code=122)

Reported-by: Selva Nair <selva.nair@gmail.com>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/syshead.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Frank Lichtenheld Feb. 10, 2023, 5:09 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 03:31:59PM +0200, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> Socket is a handle on Windows, which is usually logged in hex.
> Also an interesting value is INVALID_SOCKET, which is ~0.
> 
> PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like
> 
>   2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle
> annettu data-alue on liian pieni.   (fd=18446744073709551615,code=122)
> 
> PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer:
> 
>   2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle
> annettu data-alue on liian pieni.   (fd=ffffffffffffffff,code=122)
> 

Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Regards,
Antonio Quartulli Feb. 10, 2023, 7:19 p.m. UTC | #2 
Hi,

On 10/02/2023 14:31, Lev Stipakov wrote:
> From: Lev Stipakov <lev@openvpn.net>
> 
> Socket is a handle on Windows, which is usually logged in hex.
> Also an interesting value is INVALID_SOCKET, which is ~0.
> 
> PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like
> 
>    2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle
> annettu data-alue on liian pieni.   (fd=18446744073709551615,code=122)
> 
> PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer:
> 
>    2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle
> annettu data-alue on liian pieni.   (fd=ffffffffffffffff,code=122)
> 
> Reported-by: Selva Nair <selva.nair@gmail.com>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>

I also discussed this with Lev and, despite this being different from 
what we do in the *nix world (where decimal representations make sense 
for file descriptors), it seems to be the right hting to do on Windows 
when using HANDLEs (shrug).

Acked-by: Antonio Quartulli <a@unstable.cc>
Selva Nair Feb. 10, 2023, 7:54 p.m. UTC | #3 
Hi

On Fri, Feb 10, 2023 at 2:21 PM Antonio Quartulli <a@unstable.cc> wrote:

> Hi,
>
> On 10/02/2023 14:31, Lev Stipakov wrote:
> > From: Lev Stipakov <lev@openvpn.net>
> >
> > Socket is a handle on Windows, which is usually logged in hex.
> > Also an interesting value is INVALID_SOCKET, which is ~0.
> >
> > PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like
> >
> >    2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle
> > annettu data-alue on liian pieni.   (fd=18446744073709551615,code=122)
> >
> > PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer:
> >
> >    2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle
> > annettu data-alue on liian pieni.   (fd=ffffffffffffffff,code=122)
> >
> > Reported-by: Selva Nair <selva.nair@gmail.com>
> > Signed-off-by: Lev Stipakov <lev@openvpn.net>
>
> I also discussed this with Lev and, despite this being different from
> what we do in the *nix world (where decimal representations make sense
> for file descriptors), it seems to be the right hting to do on Windows
> when using HANDLEs (shrug).
>

Windows kernel handles are very much "like" fds though the handle table
also covers processes, threads,  semaphores etc. They are not pointers, are
generally smallish numbers or ~0 (== -1). But not as small as fds in the
Unix/Linux world --- mainly because several objects share the name space
and they have to be multiples of 4 for some arcane reason.

The problem here was not printing as "decimal" but as "unsigned decimal".
Use of the latter would have caused the same issue on Linux too.

Using hex is a compromise here as handles are defined as "unsigned ints" of
the same size as pointers. So PRIdPTR somehow looks improper though I would
have chosen that.

Selva
Antonio Quartulli Feb. 10, 2023, 10:07 p.m. UTC | #4 
Hi,

On 10/02/2023 20:54, Selva Nair wrote:
> 
>     I also discussed this with Lev and, despite this being different from
>     what we do in the *nix world (where decimal representations make sense
>     for file descriptors), it seems to be the right hting to do on Windows
>     when using HANDLEs (shrug).
> 
> 
> Windows kernel handles are very much "like" fds though the handle table 
> also covers processes, threads,  semaphores etc. They are not pointers, 
> are generally smallish numbers or ~0 (== -1). But not as small as fds in 
> the Unix/Linux world --- mainly because several objects share the name 
> space and they have to be multiples of 4 for some arcane reason.
> 
> The problem here was not printing as "decimal" but as "unsigned 
> decimal". Use of the latter would have caused the same issue on Linux too.
> 
> Using hex is a compromise here as handles are defined as "unsigned ints" 
> of the same size as pointers. So PRIdPTR somehow looks improper though I 
> would have chosen that.

Given your explanation then it sounds like we should truly use PRIdPTR.
Wouldn't you agree Lev?

Cheers,

> 
> Selva
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Arne Schwabe Feb. 14, 2023, 11:26 a.m. UTC | #5 
Am 10.02.23 um 23:07 schrieb Antonio Quartulli:
> Hi,
> 
> On 10/02/2023 20:54, Selva Nair wrote:
>>
>>     I also discussed this with Lev and, despite this being different from
>>     what we do in the *nix world (where decimal representations make 
>> sense
>>     for file descriptors), it seems to be the right hting to do on 
>> Windows
>>     when using HANDLEs (shrug).
>>
>>
>> Windows kernel handles are very much "like" fds though the handle 
>> table also covers processes, threads,  semaphores etc. They are not 
>> pointers, are generally smallish numbers or ~0 (== -1). But not as 
>> small as fds in the Unix/Linux world --- mainly because several 
>> objects share the name space and they have to be multiples of 4 for 
>> some arcane reason.
>>
>> The problem here was not printing as "decimal" but as "unsigned 
>> decimal". Use of the latter would have caused the same issue on Linux 
>> too.
>>
>> Using hex is a compromise here as handles are defined as "unsigned 
>> ints" of the same size as pointers. So PRIdPTR somehow looks improper 
>> though I would have chosen that.
> 
> Given your explanation then it sounds like we should truly use PRIdPTR.
> Wouldn't you agree Lev?

Not really. The type is still an unsigned type and while -1 is used to 
denote an invalid HANDLE, the type itself is unsigned, so using a signed 
printf format specifier like PRIdPTR will probably give us a warning 
about using about signed vs unsigned.

Arne
Gert Doering Feb. 14, 2023, 1:11 p.m. UTC | #6 
I'm so not testing this... but I do not need to, the change is trivial
(ok, I cheated, I did test-compile on MinGW).

Your patch has been applied to the master branch.

commit 6731314a82d1a3c76b5497749985ee20c0c7d8eb (master)
commit 8e3331a901dbececc8622e97ed0592ddadf56996 (release/2.6)
Author: Lev Stipakov
Date:   Fri Feb 10 15:31:59 2023 +0200

     Improve format specifier for socket handle in Windows

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20230210133159.1336-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26220.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
index 12ccf2f4..53359225 100644
--- a/src/openvpn/syshead.h
+++ b/src/openvpn/syshead.h
@@ -441,7 +441,7 @@  typedef unsigned short sa_family_t;
  */
 #ifdef _WIN32
 #define SOCKET_UNDEFINED (INVALID_SOCKET)
-#define SOCKET_PRINTF "%" PRIuPTR
+#define SOCKET_PRINTF "%" PRIxPTR
 typedef SOCKET socket_descriptor_t;
 #else
 #define SOCKET_UNDEFINED (-1)