| Message ID | 20220503002840.295-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Fix M_ERRNO behavior on Windows | expand |
Hi, Thanks for the changes. Looks good. On Tue, May 3, 2022 at 9:12 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > We use M_ERRNO flag in logging to display error code > and error message. This has been broken on Windows, > where we use error code from GetLastError() and > error description from strerror(). strerror() expects > C runtime error code, which is quite different from > last error code from WinAPI call. As a result, we got > incorrect error description. > > The ultimate fix would be introducing another flag > for WinAPI errors, like M_WINERR and use either that or > M_ERRNO depends on context. However, the change would be > quite intrusive and in some cases it is hard to say which > one to use without looking into internals. > > Instead we stick to M_ERRNO and in Windows case we > first try to obtain error code from GetLastError() and > if it returns ERROR_SUCCESS (which is 0), we assume that > we have C runtime error and use errno. To get error > description we use strerror_win32() with GetLastError() > and strerror() with errno. > > strerror_win32() uses FormatMessage() internally, which > is the right way to get WinAPI error description. > --- > v2: > - removed WSA error printing, to be implemented in a follow-up patch > - added missing crt error fallback to x_check_status() and > main_io_error() > - fixed "network unreachable" detection > > src/openvpn/error.c | 34 +++++++++++++++++++++++++++------- > src/openvpn/error.h | 39 +++++++++++++++++++++++++++++---------- > src/openvpn/forward.c | 9 ++++++++- > src/openvpn/manage.c | 5 +++-- > src/openvpn/platform.c | 2 +- > src/openvpn/tun.h | 4 ++-- > 6 files changed, 70 insertions(+), 23 deletions(-) > > diff --git a/src/openvpn/error.c b/src/openvpn/error.c > index 603d6c63..1b7f5cde 100644 > --- a/src/openvpn/error.c > +++ b/src/openvpn/error.c > @@ -220,6 +220,18 @@ x_msg(const unsigned int flags, const char *format, > ...) > va_end(arglist); > } > > +static const char* > +openvpn_strerror(int err, bool crt_error, struct gc_arena *gc) > +{ > +#ifdef _WIN32 > + if (!crt_error) > + { > + return strerror_win32(err, gc); > + } > +#endif > + return strerror(err); > +} > + > void > x_msg_va(const unsigned int flags, const char *format, va_list arglist) > { > @@ -242,7 +254,8 @@ x_msg_va(const unsigned int flags, const char *format, > va_list arglist) > return; > } > > - e = openvpn_errno(); > + bool crt_error = false; > + e = openvpn_errno_maybe_crt(&crt_error); > > /* > * Apply muting filter. > @@ -264,7 +277,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, crt_error, &gc), e); > SWAP; > } > > @@ -643,7 +656,6 @@ x_check_status(int status, > struct link_socket *sock, > struct tuntap *tt) > { > - const int my_errno = openvpn_errno(); > const char *extended_msg = NULL; > > msg(x_cs_verbose_level, "%s %s returned %d", > @@ -666,26 +678,34 @@ x_check_status(int status, > sock->info.mtu_changed = true; > } > } > -#elif defined(_WIN32) > +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */ > + > +#ifdef _WIN32 > /* get possible driver error from TAP-Windows driver */ > if (tuntap_defined(tt)) > { > extended_msg = tap_win_getinfo(tt, &gc); > } > #endif > - if (!ignore_sys_error(my_errno)) > + > + bool crt_error = false; > + int my_errno = openvpn_errno_maybe_crt(&crt_error); > + > + if (!ignore_sys_error(my_errno, crt_error)) > { > if (extended_msg) > { > msg(x_cs_info_level, "%s %s [%s]: %s (fd=%d,code=%d)", > description, > sock ? proto2ascii(sock->info.proto, sock->info.af, > true) : "", > - extended_msg, strerror(my_errno), sock ? sock->sd : > -1, my_errno); > + extended_msg, openvpn_strerror(my_errno, crt_error, > &gc), > + sock ? sock->sd : -1, my_errno); > } > else > { > msg(x_cs_info_level, "%s %s: %s (fd=%d,code=%d)", > description, > sock ? proto2ascii(sock->info.proto, sock->info.af, > true) : "", > - strerror(my_errno), sock ? sock->sd : -1, my_errno); > + openvpn_strerror(my_errno, crt_error, &gc), > + sock ? sock->sd : -1, my_errno); > } > > if (x_cs_err_delay_ms) > diff --git a/src/openvpn/error.h b/src/openvpn/error.h > index ad7defe8..be8d97e5 100644 > --- a/src/openvpn/error.h > +++ b/src/openvpn/error.h > @@ -75,13 +75,10 @@ struct gc_arena; > /* String and Error functions */ > > #ifdef _WIN32 > -#define openvpn_errno() GetLastError() > -#define openvpn_strerror(e, gc) strerror_win32(e, gc) > +#define openvpn_errno() GetLastError() > const char *strerror_win32(DWORD errnum, struct gc_arena *gc); > - > #else > -#define openvpn_errno() errno > -#define openvpn_strerror(x, gc) strerror(x) > +#define openvpn_errno() errno > #endif > > /* > @@ -352,20 +349,22 @@ msg_get_virtual_output(void) > * which can be safely ignored. > */ > static inline bool > -ignore_sys_error(const int err) > +ignore_sys_error(const int err, bool crt_error) > { > - /* I/O operation pending */ > #ifdef _WIN32 > - if (err == WSAEWOULDBLOCK || err == WSAEINVAL) > + if (!crt_error && ((err == WSAEWOULDBLOCK || err == WSAEINVAL))) > { > return true; > } > #else > - if (err == EAGAIN) > + crt_error = true; > +#endif > + > + /* I/O operation pending */ > + if (crt_error && (err == EAGAIN)) > { > return true; > } > -#endif > > #if 0 /* if enabled, suppress ENOBUFS errors */ > #ifdef ENOBUFS > @@ -387,6 +386,26 @@ nonfatal(const unsigned int err) > return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err; > } > > +static inline int > +openvpn_errno_maybe_crt(bool *crt_error) > +{ > + int err = 0; > + *crt_error = false; > +#ifdef _WIN32 > + err = GetLastError(); > + if (err == ERROR_SUCCESS) > + { > + /* error is likely C runtime */ > + *crt_error = true; > + err = errno; > + } > +#else > + *crt_error = true; > + err = errno; > +#endif > + return err; > +} > + > #include "errlevel.h" > > #endif /* ifndef ERROR_H */ > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index 8930e578..04828a5c 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -1660,7 +1660,14 @@ process_outgoing_link(struct context *c) > } > > /* for unreachable network and "connecting" state switch to the > next host */ > - if (size < 0 && ENETUNREACH == error_code && c->c2.tls_multi > + > + bool unreachable = error_code == > +#ifdef _WIN32 > + WSAENETUNREACH; > +#else > + ENETUNREACH; > +#endif > + if (size < 0 && unreachable && c->c2.tls_multi > && !tls_initial_packet_received(c->c2.tls_multi) && > c->options.mode == MODE_POINT_TO_POINT) > { > msg(M_INFO, "Network unreachable, restarting"); > diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c > index 9b03b057..036658b1 100644 > --- a/src/openvpn/manage.c > +++ b/src/openvpn/manage.c > @@ -2008,9 +2008,10 @@ man_process_command(struct management *man, const > char *line) > static bool > man_io_error(struct management *man, const char *prefix) > { > - const int err = openvpn_errno(); > + bool crt_error = false; > + int err = openvpn_errno_maybe_crt(&crt_error); > > - if (!ignore_sys_error(err)) > + if (!ignore_sys_error(err, crt_error)) > { > struct gc_arena gc = gc_new(); > msg(D_MANAGEMENT, "MANAGEMENT: TCP %s error: %s", prefix, > diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c > index 61afee83..ae1678db 100644 > --- a/src/openvpn/platform.c > +++ b/src/openvpn/platform.c > @@ -532,7 +532,7 @@ platform_test_file(const char *filename) > } > else > { > - if (openvpn_errno() == EACCES) > + if (errno == EACCES) > { > msg( M_WARN | M_ERRNO, "Could not access file '%s'", > filename); > } > diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h > index 3a7314c5..4bc35916 100644 > --- a/src/openvpn/tun.h > +++ b/src/openvpn/tun.h > @@ -446,7 +446,7 @@ tuntap_stop(int status) > */ > if (status < 0) > { > - return openvpn_errno() == ERROR_FILE_NOT_FOUND; > + return GetLastError() == ERROR_FILE_NOT_FOUND; > } > return false; > } > @@ -459,7 +459,7 @@ tuntap_abort(int status) > */ > if (status < 0) > { > - return openvpn_errno() == ERROR_OPERATION_ABORTED; > + return GetLastError() == ERROR_OPERATION_ABORTED; > } > return false; > } > Acked-by: Selva Nair <selva.nair@gmail.com> <div dir="ltr"><div>Hi,</div><div><br></div><div>Thanks for the changes. Looks good.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 3, 2022 at 9:12 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> 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">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> <br> We use M_ERRNO flag in logging to display error code<br> and error message. This has been broken on Windows,<br> where we use error code from GetLastError() and<br> error description from strerror(). strerror() expects<br> C runtime error code, which is quite different from<br> last error code from WinAPI call. As a result, we got<br> incorrect error description.<br> <br> The ultimate fix would be introducing another flag<br> for WinAPI errors, like M_WINERR and use either that or<br> M_ERRNO depends on context. However, the change would be<br> quite intrusive and in some cases it is hard to say which<br> one to use without looking into internals.<br> <br> Instead we stick to M_ERRNO and in Windows case we<br> first try to obtain error code from GetLastError() and<br> if it returns ERROR_SUCCESS (which is 0), we assume that<br> we have C runtime error and use errno. To get error<br> description we use strerror_win32() with GetLastError()<br> and strerror() with errno.<br> <br> strerror_win32() uses FormatMessage() internally, which<br> is the right way to get WinAPI error description.<br> ---<br> v2:<br> - removed WSA error printing, to be implemented in a follow-up patch<br> - added missing crt error fallback to x_check_status() and<br> main_io_error()<br> - fixed "network unreachable" detection<br> <br> src/openvpn/error.c | 34 +++++++++++++++++++++++++++-------<br> src/openvpn/error.h | 39 +++++++++++++++++++++++++++++----------<br> src/openvpn/forward.c | 9 ++++++++-<br> src/openvpn/manage.c | 5 +++--<br> src/openvpn/platform.c | 2 +-<br> src/openvpn/tun.h | 4 ++--<br> 6 files changed, 70 insertions(+), 23 deletions(-)<br> <br> diff --git a/src/openvpn/error.c b/src/openvpn/error.c<br> index 603d6c63..1b7f5cde 100644<br> --- a/src/openvpn/error.c<br> +++ b/src/openvpn/error.c<br> @@ -220,6 +220,18 @@ x_msg(const unsigned int flags, const char *format, ...)<br> va_end(arglist);<br> }<br> <br> +static const char*<br> +openvpn_strerror(int err, bool crt_error, struct gc_arena *gc)<br> +{<br> +#ifdef _WIN32<br> + if (!crt_error)<br> + {<br> + return strerror_win32(err, gc);<br> + }<br> +#endif<br> + return strerror(err);<br> +}<br> +<br> void<br> x_msg_va(const unsigned int flags, const char *format, va_list arglist)<br> {<br> @@ -242,7 +254,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)<br> return;<br> }<br> <br> - e = openvpn_errno();<br> + bool crt_error = false;<br> + e = openvpn_errno_maybe_crt(&crt_error);<br> <br> /*<br> * Apply muting filter.<br> @@ -264,7 +277,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)<br> if ((flags & M_ERRNO) && e)<br> {<br> openvpn_snprintf(m2, ERR_BUF_SIZE, "%s: %s (errno=%d)",<br> - m1, strerror(e), e);<br> + m1, openvpn_strerror(e, crt_error, &gc), e);<br> SWAP;<br> }<br> <br> @@ -643,7 +656,6 @@ x_check_status(int status,<br> struct link_socket *sock,<br> struct tuntap *tt)<br> {<br> - const int my_errno = openvpn_errno();<br> const char *extended_msg = NULL;<br> <br> msg(x_cs_verbose_level, "%s %s returned %d",<br> @@ -666,26 +678,34 @@ x_check_status(int status,<br> sock->info.mtu_changed = true;<br> }<br> }<br> -#elif defined(_WIN32)<br> +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */<br> +<br> +#ifdef _WIN32<br> /* get possible driver error from TAP-Windows driver */<br> if (tuntap_defined(tt))<br> {<br> extended_msg = tap_win_getinfo(tt, &gc);<br> }<br> #endif<br> - if (!ignore_sys_error(my_errno))<br> +<br> + bool crt_error = false;<br> + int my_errno = openvpn_errno_maybe_crt(&crt_error);<br> +<br> + if (!ignore_sys_error(my_errno, crt_error))<br> {<br> if (extended_msg)<br> {<br> msg(x_cs_info_level, "%s %s [%s]: %s (fd=%d,code=%d)", description,<br> sock ? proto2ascii(sock->info.proto, sock-><a href="http://info.af" rel="noreferrer" target="_blank">info.af</a>, true) : "",<br> - extended_msg, strerror(my_errno), sock ? sock->sd : -1, my_errno);<br> + extended_msg, openvpn_strerror(my_errno, crt_error, &gc),<br> + sock ? sock->sd : -1, my_errno);<br> }<br> else<br> {<br> msg(x_cs_info_level, "%s %s: %s (fd=%d,code=%d)", description,<br> sock ? proto2ascii(sock->info.proto, sock-><a href="http://info.af" rel="noreferrer" target="_blank">info.af</a>, true) : "",<br> - strerror(my_errno), sock ? sock->sd : -1, my_errno);<br> + openvpn_strerror(my_errno, crt_error, &gc),<br> + sock ? sock->sd : -1, my_errno);<br> }<br> <br> if (x_cs_err_delay_ms)<br> diff --git a/src/openvpn/error.h b/src/openvpn/error.h<br> index ad7defe8..be8d97e5 100644<br> --- a/src/openvpn/error.h<br> +++ b/src/openvpn/error.h<br> @@ -75,13 +75,10 @@ struct gc_arena;<br> /* String and Error functions */<br> <br> #ifdef _WIN32<br> -#define openvpn_errno() GetLastError()<br> -#define openvpn_strerror(e, gc) strerror_win32(e, gc)<br> +#define openvpn_errno() GetLastError()<br> const char *strerror_win32(DWORD errnum, struct gc_arena *gc);<br> -<br> #else<br> -#define openvpn_errno() errno<br> -#define openvpn_strerror(x, gc) strerror(x)<br> +#define openvpn_errno() errno<br> #endif<br> <br> /*<br> @@ -352,20 +349,22 @@ msg_get_virtual_output(void)<br> * which can be safely ignored.<br> */<br> static inline bool<br> -ignore_sys_error(const int err)<br> +ignore_sys_error(const int err, bool crt_error)<br> {<br> - /* I/O operation pending */<br> #ifdef _WIN32<br> - if (err == WSAEWOULDBLOCK || err == WSAEINVAL)<br> + if (!crt_error && ((err == WSAEWOULDBLOCK || err == WSAEINVAL)))<br> {<br> return true;<br> }<br> #else<br> - if (err == EAGAIN)<br> + crt_error = true;<br> +#endif<br> +<br> + /* I/O operation pending */<br> + if (crt_error && (err == EAGAIN))<br> {<br> return true;<br> }<br> -#endif<br> <br> #if 0 /* if enabled, suppress ENOBUFS errors */<br> #ifdef ENOBUFS<br> @@ -387,6 +386,26 @@ nonfatal(const unsigned int err)<br> return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err;<br> }<br> <br> +static inline int<br> +openvpn_errno_maybe_crt(bool *crt_error)<br> +{<br> + int err = 0;<br> + *crt_error = false;<br> +#ifdef _WIN32<br> + err = GetLastError();<br> + if (err == ERROR_SUCCESS)<br> + {<br> + /* error is likely C runtime */<br> + *crt_error = true;<br> + err = errno;<br> + }<br> +#else<br> + *crt_error = true;<br> + err = errno;<br> +#endif<br> + return err;<br> +}<br> +<br> #include "errlevel.h"<br> <br> #endif /* ifndef ERROR_H */<br> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c<br> index 8930e578..04828a5c 100644<br> --- a/src/openvpn/forward.c<br> +++ b/src/openvpn/forward.c<br> @@ -1660,7 +1660,14 @@ process_outgoing_link(struct context *c)<br> }<br> <br> /* for unreachable network and "connecting" state switch to the next host */<br> - if (size < 0 && ENETUNREACH == error_code && c->c2.tls_multi<br> +<br> + bool unreachable = error_code ==<br> +#ifdef _WIN32<br> + WSAENETUNREACH;<br> +#else<br> + ENETUNREACH;<br> +#endif<br> + if (size < 0 && unreachable && c->c2.tls_multi<br> && !tls_initial_packet_received(c->c2.tls_multi) && c->options.mode == MODE_POINT_TO_POINT)<br> {<br> msg(M_INFO, "Network unreachable, restarting");<br> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c<br> index 9b03b057..036658b1 100644<br> --- a/src/openvpn/manage.c<br> +++ b/src/openvpn/manage.c<br> @@ -2008,9 +2008,10 @@ man_process_command(struct management *man, const char *line)<br> static bool<br> man_io_error(struct management *man, const char *prefix)<br> {<br> - const int err = openvpn_errno();<br> + bool crt_error = false;<br> + int err = openvpn_errno_maybe_crt(&crt_error);<br> <br> - if (!ignore_sys_error(err))<br> + if (!ignore_sys_error(err, crt_error))<br> {<br> struct gc_arena gc = gc_new();<br> msg(D_MANAGEMENT, "MANAGEMENT: TCP %s error: %s", prefix,<br> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c<br> index 61afee83..ae1678db 100644<br> --- a/src/openvpn/platform.c<br> +++ b/src/openvpn/platform.c<br> @@ -532,7 +532,7 @@ platform_test_file(const char *filename)<br> }<br> else<br> {<br> - if (openvpn_errno() == EACCES)<br> + if (errno == EACCES)<br> {<br> msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename);<br> }<br> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h<br> index 3a7314c5..4bc35916 100644<br> --- a/src/openvpn/tun.h<br> +++ b/src/openvpn/tun.h<br> @@ -446,7 +446,7 @@ tuntap_stop(int status)<br> */<br> if (status < 0)<br> {<br> - return openvpn_errno() == ERROR_FILE_NOT_FOUND;<br> + return GetLastError() == ERROR_FILE_NOT_FOUND;<br> }<br> return false;<br> }<br> @@ -459,7 +459,7 @@ tuntap_abort(int status)<br> */<br> if (status < 0)<br> {<br> - return openvpn_errno() == ERROR_OPERATION_ABORTED;<br> + return GetLastError() == ERROR_OPERATION_ABORTED;<br> }<br> return false;<br> }<br></blockquote><div><br></div><div>Acked-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></div></div></div>
Your patch has been applied to the master branch.
I've added the S-O-B line according to our developer documentation.
I have come late to the party, but really, code like this should be
avoided:
+ bool unreachable = error_code ==
+#ifdef _WIN32
+ WSAENETUNREACH;
+#else
+ ENETUNREACH;
+#endif
what is wrong writing this as follows?
+#ifdef _WIN32
+ bool unreachable = (error_code == WSAENETUNREACH);
+#else
+ bool unreachable = (error_code == ENETUNREACH);
+#endif
one line *less*, and very clear what happens in both branches, not
"old style openvpn contortions with #ifdef in the middle of an
expression".
Also, assignments of the type "a = b == c;" should use brackets, always
- even if the C compiler understands what this code does, humans need
extra brain cycles to make sense of it.
commit 54800aa975418fe3570f3206a5f9b277dc59bd47
Author: Lev Stipakov
Date: Tue May 3 03:28:40 2022 +0300
Fix M_ERRNO behavior on Windows
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20220503002840.295-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24270.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Thu, May 12, 2022 at 08:32:49AM +0200, Gert Doering wrote: > I have come late to the party, but really, code like this should be > avoided: > > + bool unreachable = error_code == > +#ifdef _WIN32 > + WSAENETUNREACH; > +#else > + ENETUNREACH; > +#endif Even worse, this is also uncrustify-noncompliant... and I forgot to double check. I have fixed the whitespace errors introduced in the M_ERRNO patch in commit 28557b50c6c9, as attached (and already pushed). gert
Hi, On Tue, May 03, 2022 at 03:28:40AM +0300, Lev Stipakov wrote: > From: Lev Stipakov <lev@openvpn.net> > > We use M_ERRNO flag in logging to display error code > and error message. This has been broken on Windows, > where we use error code from GetLastError() and > error description from strerror(). strerror() expects > C runtime error code, which is quite different from > last error code from WinAPI call. As a result, we got > incorrect error description. This patch breaks extended socket error reporting. In the git tree, we have those two right next to each other commit 54800aa975418fe3570f3206a5f9b277dc59bd47 Author: Lev Stipakov <lev@openvpn.net> Date: Tue May 3 03:28:40 2022 +0300 Fix M_ERRNO behavior on Windows -> broken commit 043c67f36342969cd171d24c70ee6b62ebc95fee Author: Gert Doering <gert@greenie.muc.de> Date: Tue Feb 22 15:35:14 2022 +0100 Implement --mtu-disc for IPv6 UDP sockets. -> works Testing is easy: point openvpn on a Linux system(!) to an unreachable destination (--verb 5 used here, but 3 should be sufficient)). With 043c67f, this gives 2022-07-22 18:07:59 us=599924 UDPv6 link remote: [AF_INET6]2001:608:2:a::253:1196 W2022-07-22 18:07:59 us=628976 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111) W2022-07-22 18:08:02 us=168862 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111) W2022-07-22 18:08:07 us=228896 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111) with 54800aa9754, this becomes 2022-07-22 18:14:16 us=271797 UDPv6 link remote: [AF_INET6]2001:608:2:a::253:1196 WWWWW^C2022-07-22 18:14:46 us=706438 event_wait : Interrupted system call (fd=-1,code=4) ("nothing nothing nothing ctrl-c") I can see that the patch at least touches code close to it... +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */ ... so it would be nice if someone could have a look what this patch is trampling on that it shouldn't... gert
On Fri, Jul 22, 2022 at 12:17 PM Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Tue, May 03, 2022 at 03:28:40AM +0300, Lev Stipakov wrote: > > From: Lev Stipakov <lev@openvpn.net> > > > > We use M_ERRNO flag in logging to display error code > > and error message. This has been broken on Windows, > > where we use error code from GetLastError() and > > error description from strerror(). strerror() expects > > C runtime error code, which is quite different from > > last error code from WinAPI call. As a result, we got > > incorrect error description. > > This patch breaks extended socket error reporting. > > In the git tree, we have those two right next to each other > > commit 54800aa975418fe3570f3206a5f9b277dc59bd47 > Author: Lev Stipakov <lev@openvpn.net> > Date: Tue May 3 03:28:40 2022 +0300 > > Fix M_ERRNO behavior on Windows > > -> broken > > commit 043c67f36342969cd171d24c70ee6b62ebc95fee > Author: Gert Doering <gert@greenie.muc.de> > Date: Tue Feb 22 15:35:14 2022 +0100 > > Implement --mtu-disc for IPv6 UDP sockets. > > -> works > > > Testing is easy: point openvpn on a Linux system(!) to an unreachable > destination (--verb 5 used here, but 3 should be sufficient)). > > With 043c67f, this gives > > 2022-07-22 18:07:59 us=599924 UDPv6 link remote: > [AF_INET6]2001:608:2:a::253:1196 > W2022-07-22 18:07:59 us=628976 read UDPv6 [ECONNREFUSED]: Connection > refused (fd=3,code=111) > W2022-07-22 18:08:02 us=168862 read UDPv6 [ECONNREFUSED]: Connection > refused (fd=3,code=111) > W2022-07-22 18:08:07 us=228896 read UDPv6 [ECONNREFUSED]: Connection > refused (fd=3,code=111) > > with 54800aa9754, this becomes > > 2022-07-22 18:14:16 us=271797 UDPv6 link remote: > [AF_INET6]2001:608:2:a::253:1196 > WWWWW^C2022-07-22 18:14:46 us=706438 event_wait : Interrupted system call > (fd=-1,code=4) > > ("nothing nothing nothing ctrl-c") > This one is tricky -- though the patch appeared to be a no-op for non-Windows, in error.c:x_check_status(), it did move down the line my_errno = openvpn_errno() and thus potentially lose the original error. At that time it looked like a good move (pun intended), though. So much for C99 style.. Especially, before outputting the message we call format_extended_socket_error() which calls recvmsg() which can easily return EAGAIN in errno. To top it, EAGAIN is completely ignored by ignore_sys_error(), so we get nothing printed. I think moving those line back up to the start of the function should fix this Selva > > I can see that the patch at least touches code close to it... > > +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */ > > ... so it would be nice if someone could have a look what this patch > is trampling on that it shouldn't... > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never > doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh > Mistress > > Gert Doering - Munich, Germany > gert@greenie.muc.de > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 22, 2022 at 12:17 PM Gert Doering <<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>> 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">Hi,<br> <br> On Tue, May 03, 2022 at 03:28:40AM +0300, Lev Stipakov wrote:<br> > From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> > <br> > We use M_ERRNO flag in logging to display error code<br> > and error message. This has been broken on Windows,<br> > where we use error code from GetLastError() and<br> > error description from strerror(). strerror() expects<br> > C runtime error code, which is quite different from<br> > last error code from WinAPI call. As a result, we got<br> > incorrect error description.<br> <br> This patch breaks extended socket error reporting.<br> <br> In the git tree, we have those two right next to each other<br> <br> commit 54800aa975418fe3570f3206a5f9b277dc59bd47<br> Author: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> Date: Tue May 3 03:28:40 2022 +0300<br> <br> Fix M_ERRNO behavior on Windows<br> <br> -> broken<br> <br> commit 043c67f36342969cd171d24c70ee6b62ebc95fee<br> Author: Gert Doering <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>><br> Date: Tue Feb 22 15:35:14 2022 +0100<br> <br> Implement --mtu-disc for IPv6 UDP sockets.<br> <br> -> works<br> <br> <br> Testing is easy: point openvpn on a Linux system(!) to an unreachable<br> destination (--verb 5 used here, but 3 should be sufficient)).<br> <br> With 043c67f, this gives<br> <br> 2022-07-22 18:07:59 us=599924 UDPv6 link remote: [AF_INET6]2001:608:2:a::253:1196<br> W2022-07-22 18:07:59 us=628976 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111)<br> W2022-07-22 18:08:02 us=168862 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111)<br> W2022-07-22 18:08:07 us=228896 read UDPv6 [ECONNREFUSED]: Connection refused (fd=3,code=111)<br> <br> with 54800aa9754, this becomes<br> <br> 2022-07-22 18:14:16 us=271797 UDPv6 link remote: [AF_INET6]2001:608:2:a::253:1196<br> WWWWW^C2022-07-22 18:14:46 us=706438 event_wait : Interrupted system call (fd=-1,code=4)<br> <br> ("nothing nothing nothing ctrl-c")<br></blockquote><div><br></div><div>This one is tricky -- though the patch appeared to be a no-op for non-Windows, in error.c:x_check_status(), it did move down the line my_errno = openvpn_errno() and thus potentially lose the original error. At that time it looked like a good move (pun intended), though. So much for C99 style..</div><div><br></div><div>Especially, before outputting the message we call format_extended_socket_error() which calls recvmsg() which can easily return EAGAIN in errno. To top it, EAGAIN is completely ignored by ignore_sys_error(), so we get nothing printed. I think moving those line back up to the start of the function should fix this</div><div><br></div><div>Selva</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"> <br> <br> I can see that the patch at least touches code close to it...<br> <br> +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */<br> <br> ... so it would be nice if someone could have a look what this patch<br> is trampling on that it shouldn't...<br> <br> gert<br> -- <br> "If was one thing all people took for granted, was conviction that if you <br> feed honest figures into a computer, honest figures come out. Never doubted <br> it myself till I met a computer with a sense of humor."<br> Robert A. Heinlein, The Moon is a Harsh Mistress<br> <br> Gert Doering - Munich, Germany <a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a><br> _______________________________________________<br> Openvpn-devel mailing list<br> <a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank">Openvpn-devel@lists.sourceforge.net</a><br> <a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br> </blockquote></div></div>
Hi, On Fri, Jul 22, 2022 at 04:34:40PM -0400, Selva Nair wrote: > This one is tricky -- though the patch appeared to be a no-op for > non-Windows, in error.c:x_check_status(), it did move down the line > my_errno = openvpn_errno() and thus potentially lose the original error. At > that time it looked like a good move (pun intended), though. So much for > C99 style.. ;-) Thanks for your analysis and patch. I'll stare at it a bit tomorrow and test (and then send the patch I had been working on, to get rid of the setsockopt() error message on IPv4-only sockets...) gert
diff --git a/src/openvpn/error.c b/src/openvpn/error.c index 603d6c63..1b7f5cde 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -220,6 +220,18 @@ x_msg(const unsigned int flags, const char *format, ...) va_end(arglist); } +static const char* +openvpn_strerror(int err, bool crt_error, struct gc_arena *gc) +{ +#ifdef _WIN32 + if (!crt_error) + { + return strerror_win32(err, gc); + } +#endif + return strerror(err); +} + void x_msg_va(const unsigned int flags, const char *format, va_list arglist) { @@ -242,7 +254,8 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) return; } - e = openvpn_errno(); + bool crt_error = false; + e = openvpn_errno_maybe_crt(&crt_error); /* * Apply muting filter. @@ -264,7 +277,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, crt_error, &gc), e); SWAP; } @@ -643,7 +656,6 @@ x_check_status(int status, struct link_socket *sock, struct tuntap *tt) { - const int my_errno = openvpn_errno(); const char *extended_msg = NULL; msg(x_cs_verbose_level, "%s %s returned %d", @@ -666,26 +678,34 @@ x_check_status(int status, sock->info.mtu_changed = true; } } -#elif defined(_WIN32) +#endif /* EXTENDED_SOCKET_ERROR_CAPABILITY */ + +#ifdef _WIN32 /* get possible driver error from TAP-Windows driver */ if (tuntap_defined(tt)) { extended_msg = tap_win_getinfo(tt, &gc); } #endif - if (!ignore_sys_error(my_errno)) + + bool crt_error = false; + int my_errno = openvpn_errno_maybe_crt(&crt_error); + + if (!ignore_sys_error(my_errno, crt_error)) { if (extended_msg) { msg(x_cs_info_level, "%s %s [%s]: %s (fd=%d,code=%d)", description, sock ? proto2ascii(sock->info.proto, sock->info.af, true) : "", - extended_msg, strerror(my_errno), sock ? sock->sd : -1, my_errno); + extended_msg, openvpn_strerror(my_errno, crt_error, &gc), + sock ? sock->sd : -1, my_errno); } else { msg(x_cs_info_level, "%s %s: %s (fd=%d,code=%d)", description, sock ? proto2ascii(sock->info.proto, sock->info.af, true) : "", - strerror(my_errno), sock ? sock->sd : -1, my_errno); + openvpn_strerror(my_errno, crt_error, &gc), + sock ? sock->sd : -1, my_errno); } if (x_cs_err_delay_ms) diff --git a/src/openvpn/error.h b/src/openvpn/error.h index ad7defe8..be8d97e5 100644 --- a/src/openvpn/error.h +++ b/src/openvpn/error.h @@ -75,13 +75,10 @@ struct gc_arena; /* String and Error functions */ #ifdef _WIN32 -#define openvpn_errno() GetLastError() -#define openvpn_strerror(e, gc) strerror_win32(e, gc) +#define openvpn_errno() GetLastError() const char *strerror_win32(DWORD errnum, struct gc_arena *gc); - #else -#define openvpn_errno() errno -#define openvpn_strerror(x, gc) strerror(x) +#define openvpn_errno() errno #endif /* @@ -352,20 +349,22 @@ msg_get_virtual_output(void) * which can be safely ignored. */ static inline bool -ignore_sys_error(const int err) +ignore_sys_error(const int err, bool crt_error) { - /* I/O operation pending */ #ifdef _WIN32 - if (err == WSAEWOULDBLOCK || err == WSAEINVAL) + if (!crt_error && ((err == WSAEWOULDBLOCK || err == WSAEINVAL))) { return true; } #else - if (err == EAGAIN) + crt_error = true; +#endif + + /* I/O operation pending */ + if (crt_error && (err == EAGAIN)) { return true; } -#endif #if 0 /* if enabled, suppress ENOBUFS errors */ #ifdef ENOBUFS @@ -387,6 +386,26 @@ nonfatal(const unsigned int err) return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err; } +static inline int +openvpn_errno_maybe_crt(bool *crt_error) +{ + int err = 0; + *crt_error = false; +#ifdef _WIN32 + err = GetLastError(); + if (err == ERROR_SUCCESS) + { + /* error is likely C runtime */ + *crt_error = true; + err = errno; + } +#else + *crt_error = true; + err = errno; +#endif + return err; +} + #include "errlevel.h" #endif /* ifndef ERROR_H */ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 8930e578..04828a5c 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1660,7 +1660,14 @@ process_outgoing_link(struct context *c) } /* for unreachable network and "connecting" state switch to the next host */ - if (size < 0 && ENETUNREACH == error_code && c->c2.tls_multi + + bool unreachable = error_code == +#ifdef _WIN32 + WSAENETUNREACH; +#else + ENETUNREACH; +#endif + if (size < 0 && unreachable && c->c2.tls_multi && !tls_initial_packet_received(c->c2.tls_multi) && c->options.mode == MODE_POINT_TO_POINT) { msg(M_INFO, "Network unreachable, restarting"); diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 9b03b057..036658b1 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -2008,9 +2008,10 @@ man_process_command(struct management *man, const char *line) static bool man_io_error(struct management *man, const char *prefix) { - const int err = openvpn_errno(); + bool crt_error = false; + int err = openvpn_errno_maybe_crt(&crt_error); - if (!ignore_sys_error(err)) + if (!ignore_sys_error(err, crt_error)) { struct gc_arena gc = gc_new(); msg(D_MANAGEMENT, "MANAGEMENT: TCP %s error: %s", prefix, diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c index 61afee83..ae1678db 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -532,7 +532,7 @@ platform_test_file(const char *filename) } else { - if (openvpn_errno() == EACCES) + if (errno == EACCES) { msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename); } diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index 3a7314c5..4bc35916 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -446,7 +446,7 @@ tuntap_stop(int status) */ if (status < 0) { - return openvpn_errno() == ERROR_FILE_NOT_FOUND; + return GetLastError() == ERROR_FILE_NOT_FOUND; } return false; } @@ -459,7 +459,7 @@ tuntap_abort(int status) */ if (status < 0) { - return openvpn_errno() == ERROR_OPERATION_ABORTED; + return GetLastError() == ERROR_OPERATION_ABORTED; } return false; }
From: Lev Stipakov <lev@openvpn.net> We use M_ERRNO flag in logging to display error code and error message. This has been broken on Windows, where we use error code from GetLastError() and error description from strerror(). strerror() expects C runtime error code, which is quite different from last error code from WinAPI call. As a result, we got incorrect error description. The ultimate fix would be introducing another flag for WinAPI errors, like M_WINERR and use either that or M_ERRNO depends on context. However, the change would be quite intrusive and in some cases it is hard to say which one to use without looking into internals. Instead we stick to M_ERRNO and in Windows case we first try to obtain error code from GetLastError() and if it returns ERROR_SUCCESS (which is 0), we assume that we have C runtime error and use errno. To get error description we use strerror_win32() with GetLastError() and strerror() with errno. strerror_win32() uses FormatMessage() internally, which is the right way to get WinAPI error description. --- v2: - removed WSA error printing, to be implemented in a follow-up patch - added missing crt error fallback to x_check_status() and main_io_error() - fixed "network unreachable" detection src/openvpn/error.c | 34 +++++++++++++++++++++++++++------- src/openvpn/error.h | 39 +++++++++++++++++++++++++++++---------- src/openvpn/forward.c | 9 ++++++++- src/openvpn/manage.c | 5 +++-- src/openvpn/platform.c | 2 +- src/openvpn/tun.h | 4 ++-- 6 files changed, 70 insertions(+), 23 deletions(-)