diff mbox series

[Openvpn-devel,master+release/2.5] error.c: use correct API to get error description on Windows

Message ID 20220218124149.149-1-lstipakov@gmail.com
State Superseded
Headers show
Series [Openvpn-devel,master+release/2.5] error.c: use correct API to get error description on Windows | expand

Commit Message

Lev Stipakov Feb. 18, 2022, 1:41 a.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

On Windows we use GetLastError() to get error code. To get
error description, we must use FormatMessage() and not strerror().

Replace strerror() with openvpn_strerror() macro, which is resolved
to strerror_win32() (which calls FormatMessage) on Windows and
to strerror() on other platforms.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/error.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lev Stipakov Feb. 20, 2022, 10:22 p.m. UTC | #1 
We had a long discussion with ordex about this patch and came to the
conclusion that error printing is currently broken on Windows and
needs a proper fixing.

Why is it broken?

 - the bug that my patch fixes - we use Windows's GetLastError to get
Windows last error code and with that strerror to get C runtime error
description

 - we have code which uses M_ERRNO to get description of C runtime
errors, for example

    int fd = platform_open(filename, O_CREAT | O_TRUNC | O_WRONLY,
                           S_IRUSR | S_IWUSR);
    if (fd == -1)
    {
        msg(M_ERRNO, "Cannot open file '%s' for write", filename);
        return false;
    }

   but this doesn't really work, because msg(M_ERRNO) uses
GetLastError on Windows to get error code, and C runtime (in above
case it is _wopen) doesn't set the WinAPI's last error code.

 - we have code which uses M_ERRNO to print description of socket errors

    const ssize_t size = send(sd, buf, strlen(buf), MSG_NOSIGNAL);
    if (size != (ssize_t) strlen(buf))
    {
        msg(D_LINK_ERRORS | M_ERRNO, "send_line: TCP port write failed
on send()");
        return false;
    }

but in this case we should use WSAGetLastError on Windows and errno on
other platforms.

What we propose:

 - M_ERRNO prints only C runtime errors on all platforms and should be
only used with C runtime functions
 - We add M_WINERR which uses GetLastError and FormatMessage to print
Windows errors
 - We add M_SOCKERR, which is resolved into M_ERRNO on all platforms
except Windows and on Windows it is M_WSAERR. We use WSAGetLastError
and FormatMessage to print WSA errors. Socket functions use M_SOCKERR.

-Lev
Selva Nair Feb. 21, 2022, 5:49 a.m. UTC | #2 
Hi

On Mon, Feb 21, 2022 at 4:24 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> We had a long discussion with ordex about this patch and came to the
> conclusion that error printing is currently broken on Windows and
> needs a proper fixing.
>
>
+1


> What we propose:
>
>  - M_ERRNO prints only C runtime errors on all platforms and should be
> only used with C runtime functions
>  - We add M_WINERR which uses GetLastError and FormatMessage to print
> Windows errors
>

While this would be a cleaner fix, it also requires extensive changes and
it is not always easy to decide where to use M_ERRNO and where to use
M_WINERR. E.g., without looking into the internals of platform.c one
doesn't know whether platform_open() uses _wopen() or CreateFile().

A possible option is to continue the use of M_ERRNO on WIndows as is
(except for socket errors), continue to use GetLastError() in x_msg(), but
if/when the latter if it returns zero, try errno and strerrror(). Not ideal
but less changes and easier and transparent to the user of msg(). That
said, I haven't checked whether GetLastError() returns zero or a valid and
correct error code for C runtime errors -- without which this approach wont
work.

 - We add M_SOCKERR, which is resolved into M_ERRNO on all platforms
> except Windows and on Windows it is M_WSAERR. We use WSAGetLastError
> and FormatMessage to print WSA errors. Socket functions use M_SOCKERR.
>

Sounds good.

BTW, there are a couple of uses of strerror() in check_status() (error.c)
missed by the original patch.

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 21, 2022 at 4:24 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We had a long discussion with ordex about this patch and came to the<br>
conclusion that error printing is currently broken on Windows and<br>
needs a proper fixing.<br><br></blockquote><div><br></div><div>+1 </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
What we propose:<br>
<br>
 - M_ERRNO prints only C runtime errors on all platforms and should be<br>
only used with C runtime functions<br>
 - We add M_WINERR which uses GetLastError and FormatMessage to print<br>
Windows errors<br></blockquote><div><br></div><div>While this would be a cleaner fix, it also requires extensive changes and it is not always easy to decide where to use M_ERRNO and where to use M_WINERR. E.g., without looking into the internals of platform.c one doesn&#39;t know whether platform_open() uses _wopen() or CreateFile().</div><div><br></div><div>A possible option is to continue the use of M_ERRNO on WIndows as is (except for socket errors), continue to use GetLastError() in x_msg(), but if/when the latter if it returns zero, try errno and strerrror(). Not ideal but less changes and easier and transparent to the user of msg(). That said, I haven&#39;t checked whether GetLastError() returns zero or a valid and correct error code for C runtime errors -- without which this approach wont work.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 - We add M_SOCKERR, which is resolved into M_ERRNO on all platforms<br>
except Windows and on Windows it is M_WSAERR. We use WSAGetLastError<br>
and FormatMessage to print WSA errors. Socket functions use M_SOCKERR.<br></blockquote><div><br></div><div>Sounds good.</div><div><br></div><div>BTW, there are a couple of uses of strerror() in check_status() (error.c) missed by the original patch.</div><div><br></div><div>Selva</div></div></div>
Lev Stipakov Feb. 21, 2022, 10:30 p.m. UTC | #3 
Hi,


> While this would be a cleaner fix, it also requires extensive changes and
> it is not always easy to decide where to use M_ERRNO and where to use
> M_WINERR. E.g., without looking into the internals of platform.c one
> doesn't know whether platform_open() uses _wopen() or CreateFile().
>

True.


> A possible option is to continue the use of M_ERRNO on WIndows as is
> (except for socket errors), continue to use GetLastError() in x_msg(), but
> if/when the latter if it returns zero, try errno and strerrror(). Not ideal
> but less changes and easier and transparent to the user of msg(). That
> said, I haven't checked whether GetLastError() returns zero or a valid and
> correct error code for C runtime errors -- without which this approach wont
> work.
>

I did some tests, ::GetLastError appears to return zero or correct error
code:

    sqrt(-1);
    int err = errno; // -33 EDOM
    DWORD lasterr = GetLastError(); // 0

    _open("c:\\Temp\\notfound", _O_RDONLY);
    err = errno; // 2 ENOENT
    lasterr = GetLastError(); // 2 ERROR_FILE_NOT_FOUND

    _open("c:\\Temp\\directory", _O_RDONLY);
    err = errno; // 13 EACCES
    lasterr = GetLastError(); // 5 ERROR_ACCESS_DENIED

    malloc(10000000000);
    err = errno; // 12 ENOMEM
    lasterr = GetLastError(); // 8 ERROR_NOT_ENOUGH_MEMORY

So let's go with this approach.


> BTW, there are a couple of uses of strerror() in check_status() (error.c)
> missed by the original patch.
>

Thanks, I will address it in this "proper" fix.
diff mbox series

Patch

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index 54796d03..cb2a0db1 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -268,7 +268,7 @@  x_msg_va(const unsigned int flags, const char *format, va_list arglist)
     if ((flags & M_ERRNO) && e)
     {
         openvpn_snprintf(m2, ERR_BUF_SIZE, "%s: %s (errno=%d)",
-                         m1, strerror(e), e);
+                         m1, openvpn_strerror(e, &gc), e);
         SWAP;
     }