| Message ID | 20220222031424.406-1-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Fix M_ERRNO behavior on Windows | expand |
Hi, Sorry for the long delay in getting back to this.. On Tue, Feb 22, 2022 at 9:13 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. > > While on it, add a new flag M_SKERR to display socket > error. On Linux it is resolved into M_ERRNO and on Windows > to an conjunction of another new flag M_WSAERR and M_ERRNO. > With M_WSAERR we use WSAGetLastError() to get error code > and strerror_win32() to get error description. > WSAGetLastError() is a correct way to get WinSock error code, > according to documentation. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpn/error.c | 26 ++++++++++++++++++++++---- > src/openvpn/error.h | 11 ++++++++++- > src/openvpn/manage.c | 4 ++-- > src/openvpn/mtu.c | 2 +- > src/openvpn/platform.c | 2 +- > src/openvpn/proxy.c | 8 ++++---- > src/openvpn/ps.c | 2 +- > src/openvpn/socket.c | 20 ++++++++++---------- > src/openvpn/socket.h | 6 ++++++ > src/openvpn/socks.c | 26 +++++++++++++------------- > 10 files changed, 70 insertions(+), 37 deletions(-) > > diff --git a/src/openvpn/error.c b/src/openvpn/error.c > index b0e9a48c..5da588d9 100644 > --- a/src/openvpn/error.c > +++ b/src/openvpn/error.c > @@ -242,7 +242,25 @@ x_msg_va(const unsigned int flags, const char > *format, va_list arglist) > return; > } > > - e = openvpn_errno(); > + bool use_strerr = false; > +#ifdef _WIN32 > + if (flags & M_WSAERR) > + { > + e = WSAGetLastError(); > + } > + else > + { > + e = GetLastError(); > + if (e == ERROR_SUCCESS) > + { > + /* error is likely C runtime, fallback to errno/strerr */ > + use_strerr = true; > + e = errno; > + } > + } > +#else > + e = errno; > +#endif > > Ideally I would have liked to modify openvpn_errno() and openvpn_strerror() to do the right thing instead of adding this logic here and below. I have not worked out what exactly that would entail -- most likely those two functions would require an extra argument to keep track of the kind of error. Also there are some places openvpn_errno() is used and the result checked against POSIX error codes: in forward.c around line 2102: if (.... && ENETUNREACH == error_code && ...) where error_code = openvpn_errno() which may return WSAENETUNREACH on Windows not ENETUNREACH-- even that depends on GetLastError() and WSAGetLastError() to behave identically. Also does code like the below in socket.c do the right thing? (around line 1451 in socket.c) status = openvpn_errno(); if (status == WSAEWOULDBLOCK) .... but openvpn_errono() doesn't look for WSA errors (though it probably still works?). /* > * Apply muting filter. > @@ -264,7 +282,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, use_strerr ? strerror(e) : > openvpn_strerror(e, &gc), e); > SWAP; > } > > @@ -678,13 +696,13 @@ x_check_status(int status, > { > 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, &gc), sock ? > sock->sd : -1, my_errno); > Here my_errno = openvpn_errno() -- don't we need the fallback to POSIX errno in this case? Could this also get called when a socket error is expected? > } > 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, &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..9c3f4dfb 100644 > --- a/src/openvpn/error.h > +++ b/src/openvpn/error.h > @@ -99,9 +99,18 @@ extern int x_msg_line_num; > #define M_NONFATAL (1<<5) /* non-fatal error */ > #define M_WARN (1<<6) /* call syslog with LOG_WARNING > */ > #define M_DEBUG (1<<7) > - > #define M_ERRNO (1<<8) /* show errno description */ > > +#ifdef _WIN32 > +#define M_WSAERR (1<<9) /* WinSock errors */ > +#endif > + > +#ifdef _WIN32 > +#define M_SKERR (M_ERRNO | M_WSAERR) /* socket errors */ > +#else > +#define M_SKERR M_ERRNO > +#endif > The two ifdefs could be combined into one. The rest looks good to me. > + > #define M_NOMUTE (1<<11) /* don't do mute processing */ > #define M_NOPREFIX (1<<12) /* don't show date/time prefix */ > #define M_USAGE_SMALL (1<<13) /* fatal options error, call > usage_small */ > diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c > index 496042a6..2fa508ef 100644 > --- a/src/openvpn/manage.c > +++ b/src/openvpn/manage.c > @@ -1890,13 +1890,13 @@ man_connect(struct management *man) > #if UNIX_SOCK_SUPPORT > if (man->settings.flags & MF_UNIX_SOCK) > { > - msg(D_LINK_ERRORS | M_ERRNO, > + msg(D_LINK_ERRORS | M_SKERR, > "MANAGEMENT: connect to unix socket %s failed", > sockaddr_unix_name(&man->settings.local_unix, "NULL")); > } > else > #endif > - msg(D_LINK_ERRORS | M_ERRNO, > + msg(D_LINK_ERRORS | M_SKERR, > "MANAGEMENT: connect to %s failed", > print_sockaddr(man->settings.local->ai_addr, &gc)); > throw_signal_soft(SIGTERM, "management-connect-failed"); > diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c > index aa810f1c..45398953 100644 > --- a/src/openvpn/mtu.c > +++ b/src/openvpn/mtu.c > @@ -407,7 +407,7 @@ set_sock_extended_error_passing(int sd) > int on = 1; > if (setsockopt(sd, SOL_IP, IP_RECVERR, (void *) &on, sizeof(on))) > { > - msg(M_WARN | M_ERRNO, > + msg(M_WARN | M_SKERR, > "Note: enable extended error passing on TCP/UDP socket failed > (IP_RECVERR)"); > } > } > diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c > index 450f28ba..1aafd604 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/proxy.c b/src/openvpn/proxy.c > index ed720161..409bc4af 100644 > --- a/src/openvpn/proxy.c > +++ b/src/openvpn/proxy.c > @@ -110,7 +110,7 @@ recv_line(socket_descriptor_t sd, > { > if (verbose) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read > timeout expired"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read > timeout expired"); > } > goto error; > } > @@ -120,7 +120,7 @@ recv_line(socket_descriptor_t sd, > { > if (verbose) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read > failed on select()"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read > failed on select()"); > } > goto error; > } > @@ -133,7 +133,7 @@ recv_line(socket_descriptor_t sd, > { > if (verbose) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read > failed on recv()"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read > failed on recv()"); > } > goto error; > } > @@ -199,7 +199,7 @@ send_line(socket_descriptor_t sd, > 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()"); > + msg(D_LINK_ERRORS | M_SKERR, "send_line: TCP port write failed on > send()"); > return false; > } > return true; > diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c > index 9dca53c8..64cc1ed3 100644 > --- a/src/openvpn/ps.c > +++ b/src/openvpn/ps.c > @@ -429,7 +429,7 @@ proxy_entry_new(struct proxy_connection **list, > /* connect to port share server */ > if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) > { > - msg(M_WARN|M_ERRNO, "PORT SHARE PROXY: cannot create socket"); > + msg(M_WARN|M_SKERR, "PORT SHARE PROXY: cannot create socket"); > return false; > } > status = openvpn_connect(sd_server,(const struct sockaddr *) > &server_addr, 5, NULL); > diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c > index 0f34a5de..acfcfd65 100644 > --- a/src/openvpn/socket.c > +++ b/src/openvpn/socket.c > @@ -1143,7 +1143,7 @@ create_socket(struct link_socket *sock, struct > addrinfo *addr) > msg(M_INFO, "Using bind-dev %s", sock->bind_dev); > if (setsockopt(sock->sd, SOL_SOCKET, SO_BINDTODEVICE, > sock->bind_dev, strlen(sock->bind_dev) + 1) != 0) > { > - msg(M_WARN|M_ERRNO, "WARN: setsockopt SO_BINDTODEVICE=%s > failed", sock->bind_dev); > + msg(M_WARN|M_SKERR, "WARN: setsockopt SO_BINDTODEVICE=%s > failed", sock->bind_dev); > } > > } > @@ -1222,7 +1222,7 @@ socket_do_accept(socket_descriptor_t sd, > > if (!socket_defined(new_sd)) > { > - msg(D_LINK_ERRORS | M_ERRNO, "TCP: getpeername() failed"); > + msg(D_LINK_ERRORS | M_SKERR, "TCP: getpeername() failed"); > } > else > { > @@ -1247,7 +1247,7 @@ socket_do_accept(socket_descriptor_t sd, > > if (!socket_defined(new_sd)) > { > - msg(D_LINK_ERRORS | M_ERRNO, "TCP: accept(%d) failed", (int)sd); > + msg(D_LINK_ERRORS | M_SKERR, "TCP: accept(%d) failed", (int)sd); > } > /* only valid if we have remote_len_af!=0 */ > else if (remote_len_af && remote_len != remote_len_af) > @@ -1313,7 +1313,7 @@ socket_listen_accept(socket_descriptor_t sd, > > if (status < 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "TCP: select() failed"); > + msg(D_LINK_ERRORS | M_SKERR, "TCP: select() failed"); > } > > if (status <= 0) > @@ -1409,12 +1409,12 @@ socket_bind(socket_descriptor_t sd, > msg(M_INFO, "setsockopt(IPV6_V6ONLY=%d)", v6only); > if (setsockopt(sd, IPPROTO_IPV6, IPV6_V6ONLY, (void *) &v6only, > sizeof(v6only))) > { > - msg(M_NONFATAL|M_ERRNO, "Setting IPV6_V6ONLY=%d failed", > v6only); > + msg(M_NONFATAL | M_SKERR, "Setting IPV6_V6ONLY=%d failed", > v6only); > } > } > if (bind(sd, cur->ai_addr, cur->ai_addrlen)) > { > - msg(M_FATAL | M_ERRNO, "%s: Socket bind failed on local address > %s", > + msg(M_FATAL | M_SKERR, "%s: Socket bind failed on local address > %s", > prefix, > print_sockaddr_ex(local->ai_addr, ":", PS_SHOW_PORT, &gc)); > } > @@ -2254,7 +2254,7 @@ link_socket_close(struct link_socket *sock) > msg(D_LOW, "TCP/UDP: Closing socket"); > if (openvpn_close_socket(sock->sd)) > { > - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket failed"); > + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket failed"); > } > } > sock->sd = SOCKET_UNDEFINED; > @@ -2271,7 +2271,7 @@ link_socket_close(struct link_socket *sock) > { > if (openvpn_close_socket(sock->ctrl_sd)) > { > - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket (ctrl_sd) > failed"); > + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket (ctrl_sd) > failed"); > } > sock->ctrl_sd = SOCKET_UNDEFINED; > } > @@ -3668,7 +3668,7 @@ sockethandle_finalize(sockethandle_t sh, > /* if no error (i.e. just not finished yet), then > DON'T execute this code */ > io->iostate = IOSTATE_INITIAL; > ASSERT(ResetEvent(io->overlapped.hEvent)); > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion > error"); > + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 > I/O: Completion error"); > } > } > break; > @@ -3681,7 +3681,7 @@ sockethandle_finalize(sockethandle_t sh, > /* error return for a non-queued operation */ > SocketHandleSetLastError(sh, io->status); > ret = -1; > - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion > non-queued error"); > + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 I/O: > Completion non-queued error"); > } > else > { > diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h > index e9f1524d..a7ec827a 100644 > --- a/src/openvpn/socket.h > +++ b/src/openvpn/socket.h > @@ -301,6 +301,12 @@ SocketHandleSetInvalError(sockethandle_t sh) > sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : > WSASetLastError(WSAEINVAL); > } > > +static inline int > +SocketHandleErrorFlag(sockethandle_t sh) > +{ > + return sh.is_handle ? M_ERRNO : M_SKERR; > +} > + > #else /* ifdef _WIN32 */ > > #define openvpn_close_socket(s) close(s) > diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c > index 768bb613..10107c18 100644 > --- a/src/openvpn/socks.c > +++ b/src/openvpn/socks.c > @@ -117,7 +117,7 @@ socks_username_password_auth(struct socks_proxy_info > *p, > > if (size != strlen(to_send)) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP > port write failed on send()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP > port write failed on send()"); > goto cleanup; > } > > @@ -145,14 +145,14 @@ socks_username_password_auth(struct socks_proxy_info > *p, > /* timeout? */ > if (status == 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: > TCP port read timeout expired"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: > TCP port read timeout expired"); > goto cleanup; > } > > /* error */ > if (status < 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: > TCP port read failed on select()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: > TCP port read failed on select()"); > goto cleanup; > } > > @@ -162,7 +162,7 @@ socks_username_password_auth(struct socks_proxy_info > *p, > /* error? */ > if (size != 1) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: > TCP port read failed on recv()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: > TCP port read failed on recv()"); > goto cleanup; > } > > @@ -204,7 +204,7 @@ socks_handshake(struct socks_proxy_info *p, > size = send(sd, method_sel, sizeof(method_sel), MSG_NOSIGNAL); > if (size != sizeof(method_sel)) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write > failed on send()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port write > failed on send()"); > return false; > } > > @@ -232,14 +232,14 @@ socks_handshake(struct socks_proxy_info *p, > /* timeout? */ > if (status == 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read > timeout expired"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read > timeout expired"); > return false; > } > > /* error */ > if (status < 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read > failed on select()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read > failed on select()"); > return false; > } > > @@ -249,7 +249,7 @@ socks_handshake(struct socks_proxy_info *p, > /* error? */ > if (size != 1) > { > - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read > failed on recv()"); > + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read > failed on recv()"); > return false; > } > > @@ -342,14 +342,14 @@ recv_socks_reply(socket_descriptor_t sd, > /* timeout? */ > if (status == 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read > timeout expired"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read > timeout expired"); > return false; > } > > /* error */ > if (status < 0) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read > failed on select()"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read > failed on select()"); > return false; > } > > @@ -359,7 +359,7 @@ recv_socks_reply(socket_descriptor_t sd, > /* error? */ > if (size != 1) > { > - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read > failed on recv()"); > + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read > failed on recv()"); > return false; > } > > @@ -484,7 +484,7 @@ establish_socks_proxy_passthru(struct socks_proxy_info > *p, > const ssize_t size = send(sd, buf, 5 + len + 2, MSG_NOSIGNAL); > if ((int)size != 5 + (int)len + 2) > { > - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: > TCP port write failed on send()"); > + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: > TCP port write failed on send()"); > goto error; > } > } > @@ -527,7 +527,7 @@ establish_socks_proxy_udpassoc(struct socks_proxy_info > *p, > 10, MSG_NOSIGNAL); > if (size != 10) > { > - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: > TCP port write failed on send()"); > + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: > TCP port write failed on send()"); > goto error; > } > } > -- > All that said, it may be hard to get all error reporting fixed in one patch, so we could proceed in steps. First make openvpn_errno() and openvpn_strerror() do the right thing with GetLastError() vs errno, then fix WSA error printing, then the rest if any. Just a thought. Selva <div dir="ltr"><div>Hi,</div><div><br></div><div>Sorry for the long delay in getting back to this.. </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 22, 2022 at 9:13 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> While on it, add a new flag M_SKERR to display socket<br> error. On Linux it is resolved into M_ERRNO and on Windows<br> to an conjunction of another new flag M_WSAERR and M_ERRNO.<br> With M_WSAERR we use WSAGetLastError() to get error code<br> and strerror_win32() to get error description.<br> WSAGetLastError() is a correct way to get WinSock error code,<br> according to documentation.<br> <br> Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> ---<br> src/openvpn/error.c | 26 ++++++++++++++++++++++----<br> src/openvpn/error.h | 11 ++++++++++-<br> src/openvpn/manage.c | 4 ++--<br> src/openvpn/mtu.c | 2 +-<br> src/openvpn/platform.c | 2 +-<br> src/openvpn/proxy.c | 8 ++++----<br> src/openvpn/ps.c | 2 +-<br> src/openvpn/socket.c | 20 ++++++++++----------<br> src/openvpn/socket.h | 6 ++++++<br> src/openvpn/socks.c | 26 +++++++++++++-------------<br> 10 files changed, 70 insertions(+), 37 deletions(-)<br> <br> diff --git a/src/openvpn/error.c b/src/openvpn/error.c<br> index b0e9a48c..5da588d9 100644<br> --- a/src/openvpn/error.c<br> +++ b/src/openvpn/error.c<br> @@ -242,7 +242,25 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)<br> return;<br> }<br> <br> - e = openvpn_errno();<br> + bool use_strerr = false;<br> +#ifdef _WIN32<br> + if (flags & M_WSAERR)<br> + {<br> + e = WSAGetLastError();<br> + }<br> + else<br> + {<br> + e = GetLastError();<br> + if (e == ERROR_SUCCESS)<br> + {<br> + /* error is likely C runtime, fallback to errno/strerr */<br> + use_strerr = true;<br> + e = errno;<br> + }<br> + }<br> +#else<br> + e = errno;<br> +#endif<br> <br></blockquote><div><br></div><div>Ideally I would have liked to modify openvpn_errno() and openvpn_strerror() to do the right thing instead of adding this logic here and below. I have not worked out what exactly that would entail -- most likely those two functions would require an extra argument to keep track of the kind of error.</div><div><br></div><div>Also there are some places openvpn_errno() is used and the result checked against POSIX error codes:</div><div><div>in forward.c around line 2102:</div><div><br></div><div>if (.... && ENETUNREACH == error_code && ...)</div><div><br></div><div>where error_code = openvpn_errno() which may return WSAENETUNREACH on Windows not ENETUNREACH-- even that depends on GetLastError() and WSAGetLastError() to behave identically.</div></div><div><br></div><div>Also does code like the below in socket.c do the right thing?</div><div>(around line 1451 in socket.c)</div><div>status = openvpn_errno(); </div><div>if (status == WSAEWOULDBLOCK) ....</div><div>but openvpn_errono() doesn't look for WSA errors (though it probably still works?).</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> * Apply muting filter.<br> @@ -264,7 +282,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, use_strerr ? strerror(e) : openvpn_strerror(e, &gc), e);<br> SWAP;<br> }<br> <br> @@ -678,13 +696,13 @@ x_check_status(int status,<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, &gc), sock ? sock->sd : -1, my_errno);<br></blockquote><div><br></div><div>Here my_errno = openvpn_errno() -- don't we need the fallback to POSIX errno in this case? Could this also get called when a socket error is expected?</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"> }<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, &gc), 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..9c3f4dfb 100644<br> --- a/src/openvpn/error.h<br> +++ b/src/openvpn/error.h<br> @@ -99,9 +99,18 @@ extern int x_msg_line_num;<br> #define M_NONFATAL (1<<5) /* non-fatal error */<br> #define M_WARN (1<<6) /* call syslog with LOG_WARNING */<br> #define M_DEBUG (1<<7)<br> -<br> #define M_ERRNO (1<<8) /* show errno description */<br> <br> +#ifdef _WIN32<br> +#define M_WSAERR (1<<9) /* WinSock errors */<br> +#endif<br> +<br> +#ifdef _WIN32<br> +#define M_SKERR (M_ERRNO | M_WSAERR) /* socket errors */<br> +#else<br> +#define M_SKERR M_ERRNO<br> +#endif<br></blockquote><div><br></div><div>The two ifdefs could be combined into one.</div><div><br></div><div>The rest looks good to me.</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"> +<br> #define M_NOMUTE (1<<11) /* don't do mute processing */<br> #define M_NOPREFIX (1<<12) /* don't show date/time prefix */<br> #define M_USAGE_SMALL (1<<13) /* fatal options error, call usage_small */<br> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c<br> index 496042a6..2fa508ef 100644<br> --- a/src/openvpn/manage.c<br> +++ b/src/openvpn/manage.c<br> @@ -1890,13 +1890,13 @@ man_connect(struct management *man)<br> #if UNIX_SOCK_SUPPORT<br> if (man->settings.flags & MF_UNIX_SOCK)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO,<br> + msg(D_LINK_ERRORS | M_SKERR,<br> "MANAGEMENT: connect to unix socket %s failed",<br> sockaddr_unix_name(&man->settings.local_unix, "NULL"));<br> }<br> else<br> #endif<br> - msg(D_LINK_ERRORS | M_ERRNO,<br> + msg(D_LINK_ERRORS | M_SKERR,<br> "MANAGEMENT: connect to %s failed",<br> print_sockaddr(man->settings.local->ai_addr, &gc));<br> throw_signal_soft(SIGTERM, "management-connect-failed");<br> diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c<br> index aa810f1c..45398953 100644<br> --- a/src/openvpn/mtu.c<br> +++ b/src/openvpn/mtu.c<br> @@ -407,7 +407,7 @@ set_sock_extended_error_passing(int sd)<br> int on = 1;<br> if (setsockopt(sd, SOL_IP, IP_RECVERR, (void *) &on, sizeof(on)))<br> {<br> - msg(M_WARN | M_ERRNO,<br> + msg(M_WARN | M_SKERR,<br> "Note: enable extended error passing on TCP/UDP socket failed (IP_RECVERR)");<br> }<br> }<br> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c<br> index 450f28ba..1aafd604 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/proxy.c b/src/openvpn/proxy.c<br> index ed720161..409bc4af 100644<br> --- a/src/openvpn/proxy.c<br> +++ b/src/openvpn/proxy.c<br> @@ -110,7 +110,7 @@ recv_line(socket_descriptor_t sd,<br> {<br> if (verbose)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read timeout expired");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read timeout expired");<br> }<br> goto error;<br> }<br> @@ -120,7 +120,7 @@ recv_line(socket_descriptor_t sd,<br> {<br> if (verbose)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read failed on select()");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read failed on select()");<br> }<br> goto error;<br> }<br> @@ -133,7 +133,7 @@ recv_line(socket_descriptor_t sd,<br> {<br> if (verbose)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read failed on recv()");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read failed on recv()");<br> }<br> goto error;<br> }<br> @@ -199,7 +199,7 @@ send_line(socket_descriptor_t sd,<br> const ssize_t size = send(sd, buf, strlen(buf), MSG_NOSIGNAL);<br> if (size != (ssize_t) strlen(buf))<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "send_line: TCP port write failed on send()");<br> + msg(D_LINK_ERRORS | M_SKERR, "send_line: TCP port write failed on send()");<br> return false;<br> }<br> return true;<br> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c<br> index 9dca53c8..64cc1ed3 100644<br> --- a/src/openvpn/ps.c<br> +++ b/src/openvpn/ps.c<br> @@ -429,7 +429,7 @@ proxy_entry_new(struct proxy_connection **list,<br> /* connect to port share server */<br> if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)<br> {<br> - msg(M_WARN|M_ERRNO, "PORT SHARE PROXY: cannot create socket");<br> + msg(M_WARN|M_SKERR, "PORT SHARE PROXY: cannot create socket");<br> return false;<br> }<br> status = openvpn_connect(sd_server,(const struct sockaddr *) &server_addr, 5, NULL);<br> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c<br> index 0f34a5de..acfcfd65 100644<br> --- a/src/openvpn/socket.c<br> +++ b/src/openvpn/socket.c<br> @@ -1143,7 +1143,7 @@ create_socket(struct link_socket *sock, struct addrinfo *addr)<br> msg(M_INFO, "Using bind-dev %s", sock->bind_dev);<br> if (setsockopt(sock->sd, SOL_SOCKET, SO_BINDTODEVICE, sock->bind_dev, strlen(sock->bind_dev) + 1) != 0)<br> {<br> - msg(M_WARN|M_ERRNO, "WARN: setsockopt SO_BINDTODEVICE=%s failed", sock->bind_dev);<br> + msg(M_WARN|M_SKERR, "WARN: setsockopt SO_BINDTODEVICE=%s failed", sock->bind_dev);<br> }<br> <br> }<br> @@ -1222,7 +1222,7 @@ socket_do_accept(socket_descriptor_t sd,<br> <br> if (!socket_defined(new_sd))<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "TCP: getpeername() failed");<br> + msg(D_LINK_ERRORS | M_SKERR, "TCP: getpeername() failed");<br> }<br> else<br> {<br> @@ -1247,7 +1247,7 @@ socket_do_accept(socket_descriptor_t sd,<br> <br> if (!socket_defined(new_sd))<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "TCP: accept(%d) failed", (int)sd);<br> + msg(D_LINK_ERRORS | M_SKERR, "TCP: accept(%d) failed", (int)sd);<br> }<br> /* only valid if we have remote_len_af!=0 */<br> else if (remote_len_af && remote_len != remote_len_af)<br> @@ -1313,7 +1313,7 @@ socket_listen_accept(socket_descriptor_t sd,<br> <br> if (status < 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "TCP: select() failed");<br> + msg(D_LINK_ERRORS | M_SKERR, "TCP: select() failed");<br> }<br> <br> if (status <= 0)<br> @@ -1409,12 +1409,12 @@ socket_bind(socket_descriptor_t sd,<br> msg(M_INFO, "setsockopt(IPV6_V6ONLY=%d)", v6only);<br> if (setsockopt(sd, IPPROTO_IPV6, IPV6_V6ONLY, (void *) &v6only, sizeof(v6only)))<br> {<br> - msg(M_NONFATAL|M_ERRNO, "Setting IPV6_V6ONLY=%d failed", v6only);<br> + msg(M_NONFATAL | M_SKERR, "Setting IPV6_V6ONLY=%d failed", v6only);<br> }<br> }<br> if (bind(sd, cur->ai_addr, cur->ai_addrlen))<br> {<br> - msg(M_FATAL | M_ERRNO, "%s: Socket bind failed on local address %s",<br> + msg(M_FATAL | M_SKERR, "%s: Socket bind failed on local address %s",<br> prefix,<br> print_sockaddr_ex(local->ai_addr, ":", PS_SHOW_PORT, &gc));<br> }<br> @@ -2254,7 +2254,7 @@ link_socket_close(struct link_socket *sock)<br> msg(D_LOW, "TCP/UDP: Closing socket");<br> if (openvpn_close_socket(sock->sd))<br> {<br> - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket failed");<br> + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket failed");<br> }<br> }<br> sock->sd = SOCKET_UNDEFINED;<br> @@ -2271,7 +2271,7 @@ link_socket_close(struct link_socket *sock)<br> {<br> if (openvpn_close_socket(sock->ctrl_sd))<br> {<br> - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket (ctrl_sd) failed");<br> + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket (ctrl_sd) failed");<br> }<br> sock->ctrl_sd = SOCKET_UNDEFINED;<br> }<br> @@ -3668,7 +3668,7 @@ sockethandle_finalize(sockethandle_t sh,<br> /* if no error (i.e. just not finished yet), then DON'T execute this code */<br> io->iostate = IOSTATE_INITIAL;<br> ASSERT(ResetEvent(io->overlapped.hEvent));<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion error");<br> + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 I/O: Completion error");<br> }<br> }<br> break;<br> @@ -3681,7 +3681,7 @@ sockethandle_finalize(sockethandle_t sh,<br> /* error return for a non-queued operation */<br> SocketHandleSetLastError(sh, io->status);<br> ret = -1;<br> - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion non-queued error");<br> + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 I/O: Completion non-queued error");<br> }<br> else<br> {<br> diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h<br> index e9f1524d..a7ec827a 100644<br> --- a/src/openvpn/socket.h<br> +++ b/src/openvpn/socket.h<br> @@ -301,6 +301,12 @@ SocketHandleSetInvalError(sockethandle_t sh)<br> sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : WSASetLastError(WSAEINVAL);<br> }<br> <br> +static inline int<br> +SocketHandleErrorFlag(sockethandle_t sh)<br> +{<br> + return sh.is_handle ? M_ERRNO : M_SKERR;<br> +}<br> +<br> #else /* ifdef _WIN32 */<br> <br> #define openvpn_close_socket(s) close(s)<br> diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c<br> index 768bb613..10107c18 100644<br> --- a/src/openvpn/socks.c<br> +++ b/src/openvpn/socks.c<br> @@ -117,7 +117,7 @@ socks_username_password_auth(struct socks_proxy_info *p,<br> <br> if (size != strlen(to_send))<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port write failed on send()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port write failed on send()");<br> goto cleanup;<br> }<br> <br> @@ -145,14 +145,14 @@ socks_username_password_auth(struct socks_proxy_info *p,<br> /* timeout? */<br> if (status == 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read timeout expired");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read timeout expired");<br> goto cleanup;<br> }<br> <br> /* error */<br> if (status < 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on select()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read failed on select()");<br> goto cleanup;<br> }<br> <br> @@ -162,7 +162,7 @@ socks_username_password_auth(struct socks_proxy_info *p,<br> /* error? */<br> if (size != 1)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on recv()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read failed on recv()");<br> goto cleanup;<br> }<br> <br> @@ -204,7 +204,7 @@ socks_handshake(struct socks_proxy_info *p,<br> size = send(sd, method_sel, sizeof(method_sel), MSG_NOSIGNAL);<br> if (size != sizeof(method_sel))<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write failed on send()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port write failed on send()");<br> return false;<br> }<br> <br> @@ -232,14 +232,14 @@ socks_handshake(struct socks_proxy_info *p,<br> /* timeout? */<br> if (status == 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read timeout expired");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read timeout expired");<br> return false;<br> }<br> <br> /* error */<br> if (status < 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read failed on select()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read failed on select()");<br> return false;<br> }<br> <br> @@ -249,7 +249,7 @@ socks_handshake(struct socks_proxy_info *p,<br> /* error? */<br> if (size != 1)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read failed on recv()");<br> + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read failed on recv()");<br> return false;<br> }<br> <br> @@ -342,14 +342,14 @@ recv_socks_reply(socket_descriptor_t sd,<br> /* timeout? */<br> if (status == 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read timeout expired");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read timeout expired");<br> return false;<br> }<br> <br> /* error */<br> if (status < 0)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read failed on select()");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read failed on select()");<br> return false;<br> }<br> <br> @@ -359,7 +359,7 @@ recv_socks_reply(socket_descriptor_t sd,<br> /* error? */<br> if (size != 1)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read failed on recv()");<br> + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read failed on recv()");<br> return false;<br> }<br> <br> @@ -484,7 +484,7 @@ establish_socks_proxy_passthru(struct socks_proxy_info *p,<br> const ssize_t size = send(sd, buf, 5 + len + 2, MSG_NOSIGNAL);<br> if ((int)size != 5 + (int)len + 2)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: TCP port write failed on send()");<br> + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: TCP port write failed on send()");<br> goto error;<br> }<br> }<br> @@ -527,7 +527,7 @@ establish_socks_proxy_udpassoc(struct socks_proxy_info *p,<br> 10, MSG_NOSIGNAL);<br> if (size != 10)<br> {<br> - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: TCP port write failed on send()");<br> + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: TCP port write failed on send()");<br> goto error;<br> }<br> }<br> -- <br> </blockquote><div><br></div><div>All that said, it may be hard to get all error reporting fixed in one patch, so we could proceed in steps. First make openvpn_errno() and openvpn_strerror() do the right thing with GetLastError() vs errno, then fix WSA error printing, then the rest if any. Just a thought.</div><div><br></div><div>Selva</div></div></div>
Hi, Also there are some places openvpn_errno() is used and the result checked > against POSIX error codes: > in forward.c around line 2102: > > if (.... && ENETUNREACH == error_code && ...) > > where error_code = openvpn_errno() which may return WSAENETUNREACH on > Windows not ENETUNREACH-- even that depends on GetLastError() and > WSAGetLastError() to behave identically. > Thanks, fixed. > Also does code like the below in socket.c do the right thing? > (around line 1451 in socket.c) > status = openvpn_errno(); > if (status == WSAEWOULDBLOCK) .... > but openvpn_errono() doesn't look for WSA errors (though it probably still > works?). > Yeah we compare GetLastError result here with WSA*, but in practice that works. Here we just call openvpn_errno(), we could add WSA* support but this will make code less readable for the benefit of being more "pure", not sure it is worth it. > - extended_msg, strerror(my_errno), sock ? sock->sd : >> -1, my_errno); >> + extended_msg, openvpn_strerror(my_errno, &gc), sock >> ? sock->sd : -1, my_errno); >> > > Here my_errno = openvpn_errno() -- don't we need the fallback to POSIX > errno in this case? Could this also get called when a socket error is > expected? > You are right. I slightly refactored that part to reuse crt error fallback code which we need in 3 different places. The rest looks good to me. > As you proposed, I removed WSA error printing from this patch, it could come in a follow-up. V2 is coming. <div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Also there are some places openvpn_errno() is used and the result checked against POSIX error codes:</div><div><div>in forward.c around line 2102:</div><div><br></div><div>if (.... && ENETUNREACH == error_code && ...)</div><div><br></div><div>where error_code = openvpn_errno() which may return WSAENETUNREACH on Windows not ENETUNREACH-- even that depends on GetLastError() and WSAGetLastError() to behave identically.</div></div></div></div></blockquote><div><br></div><div>Thanks, fixed.</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"><div dir="ltr"><div class="gmail_quote"><div>Also does code like the below in socket.c do the right thing?</div><div>(around line 1451 in socket.c)</div><div>status = openvpn_errno(); </div><div>if (status == WSAEWOULDBLOCK) ....</div><div>but openvpn_errono() doesn't look for WSA errors (though it probably still works?).</div></div></div></blockquote><div><br></div><div>Yeah we compare GetLastError result here with WSA*, but in practice that works. Here we just call openvpn_errno(), we could add WSA* support but this will make code less readable for the benefit of being more "pure", not sure it is worth it.</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"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> - extended_msg, strerror(my_errno), sock ? sock->sd : -1, my_errno);<br> + extended_msg, openvpn_strerror(my_errno, &gc), sock ? sock->sd : -1, my_errno);<br></blockquote><div><br></div><div>Here my_errno = openvpn_errno() -- don't we need the fallback to POSIX errno in this case? Could this also get called when a socket error is expected?</div></div></div></blockquote><div><br></div><div>You are right. I slightly refactored that part to reuse crt error fallback code which we need in 3 different places.</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"><div dir="ltr"><div class="gmail_quote"><div>The rest looks good to me.</div></div></div></blockquote><div><br></div><div>As you proposed, I removed WSA error printing from this patch, it could come in a follow-up.</div><div> <br></div><div>V2 is coming.</div></div></div>
diff --git a/src/openvpn/error.c b/src/openvpn/error.c index b0e9a48c..5da588d9 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -242,7 +242,25 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) return; } - e = openvpn_errno(); + bool use_strerr = false; +#ifdef _WIN32 + if (flags & M_WSAERR) + { + e = WSAGetLastError(); + } + else + { + e = GetLastError(); + if (e == ERROR_SUCCESS) + { + /* error is likely C runtime, fallback to errno/strerr */ + use_strerr = true; + e = errno; + } + } +#else + e = errno; +#endif /* * Apply muting filter. @@ -264,7 +282,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, use_strerr ? strerror(e) : openvpn_strerror(e, &gc), e); SWAP; } @@ -678,13 +696,13 @@ x_check_status(int status, { 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, &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, &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..9c3f4dfb 100644 --- a/src/openvpn/error.h +++ b/src/openvpn/error.h @@ -99,9 +99,18 @@ extern int x_msg_line_num; #define M_NONFATAL (1<<5) /* non-fatal error */ #define M_WARN (1<<6) /* call syslog with LOG_WARNING */ #define M_DEBUG (1<<7) - #define M_ERRNO (1<<8) /* show errno description */ +#ifdef _WIN32 +#define M_WSAERR (1<<9) /* WinSock errors */ +#endif + +#ifdef _WIN32 +#define M_SKERR (M_ERRNO | M_WSAERR) /* socket errors */ +#else +#define M_SKERR M_ERRNO +#endif + #define M_NOMUTE (1<<11) /* don't do mute processing */ #define M_NOPREFIX (1<<12) /* don't show date/time prefix */ #define M_USAGE_SMALL (1<<13) /* fatal options error, call usage_small */ diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 496042a6..2fa508ef 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -1890,13 +1890,13 @@ man_connect(struct management *man) #if UNIX_SOCK_SUPPORT if (man->settings.flags & MF_UNIX_SOCK) { - msg(D_LINK_ERRORS | M_ERRNO, + msg(D_LINK_ERRORS | M_SKERR, "MANAGEMENT: connect to unix socket %s failed", sockaddr_unix_name(&man->settings.local_unix, "NULL")); } else #endif - msg(D_LINK_ERRORS | M_ERRNO, + msg(D_LINK_ERRORS | M_SKERR, "MANAGEMENT: connect to %s failed", print_sockaddr(man->settings.local->ai_addr, &gc)); throw_signal_soft(SIGTERM, "management-connect-failed"); diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c index aa810f1c..45398953 100644 --- a/src/openvpn/mtu.c +++ b/src/openvpn/mtu.c @@ -407,7 +407,7 @@ set_sock_extended_error_passing(int sd) int on = 1; if (setsockopt(sd, SOL_IP, IP_RECVERR, (void *) &on, sizeof(on))) { - msg(M_WARN | M_ERRNO, + msg(M_WARN | M_SKERR, "Note: enable extended error passing on TCP/UDP socket failed (IP_RECVERR)"); } } diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c index 450f28ba..1aafd604 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/proxy.c b/src/openvpn/proxy.c index ed720161..409bc4af 100644 --- a/src/openvpn/proxy.c +++ b/src/openvpn/proxy.c @@ -110,7 +110,7 @@ recv_line(socket_descriptor_t sd, { if (verbose) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read timeout expired"); + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read timeout expired"); } goto error; } @@ -120,7 +120,7 @@ recv_line(socket_descriptor_t sd, { if (verbose) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read failed on select()"); + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read failed on select()"); } goto error; } @@ -133,7 +133,7 @@ recv_line(socket_descriptor_t sd, { if (verbose) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_line: TCP port read failed on recv()"); + msg(D_LINK_ERRORS | M_SKERR, "recv_line: TCP port read failed on recv()"); } goto error; } @@ -199,7 +199,7 @@ send_line(socket_descriptor_t sd, 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()"); + msg(D_LINK_ERRORS | M_SKERR, "send_line: TCP port write failed on send()"); return false; } return true; diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c index 9dca53c8..64cc1ed3 100644 --- a/src/openvpn/ps.c +++ b/src/openvpn/ps.c @@ -429,7 +429,7 @@ proxy_entry_new(struct proxy_connection **list, /* connect to port share server */ if ((sd_server = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0) { - msg(M_WARN|M_ERRNO, "PORT SHARE PROXY: cannot create socket"); + msg(M_WARN|M_SKERR, "PORT SHARE PROXY: cannot create socket"); return false; } status = openvpn_connect(sd_server,(const struct sockaddr *) &server_addr, 5, NULL); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 0f34a5de..acfcfd65 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -1143,7 +1143,7 @@ create_socket(struct link_socket *sock, struct addrinfo *addr) msg(M_INFO, "Using bind-dev %s", sock->bind_dev); if (setsockopt(sock->sd, SOL_SOCKET, SO_BINDTODEVICE, sock->bind_dev, strlen(sock->bind_dev) + 1) != 0) { - msg(M_WARN|M_ERRNO, "WARN: setsockopt SO_BINDTODEVICE=%s failed", sock->bind_dev); + msg(M_WARN|M_SKERR, "WARN: setsockopt SO_BINDTODEVICE=%s failed", sock->bind_dev); } } @@ -1222,7 +1222,7 @@ socket_do_accept(socket_descriptor_t sd, if (!socket_defined(new_sd)) { - msg(D_LINK_ERRORS | M_ERRNO, "TCP: getpeername() failed"); + msg(D_LINK_ERRORS | M_SKERR, "TCP: getpeername() failed"); } else { @@ -1247,7 +1247,7 @@ socket_do_accept(socket_descriptor_t sd, if (!socket_defined(new_sd)) { - msg(D_LINK_ERRORS | M_ERRNO, "TCP: accept(%d) failed", (int)sd); + msg(D_LINK_ERRORS | M_SKERR, "TCP: accept(%d) failed", (int)sd); } /* only valid if we have remote_len_af!=0 */ else if (remote_len_af && remote_len != remote_len_af) @@ -1313,7 +1313,7 @@ socket_listen_accept(socket_descriptor_t sd, if (status < 0) { - msg(D_LINK_ERRORS | M_ERRNO, "TCP: select() failed"); + msg(D_LINK_ERRORS | M_SKERR, "TCP: select() failed"); } if (status <= 0) @@ -1409,12 +1409,12 @@ socket_bind(socket_descriptor_t sd, msg(M_INFO, "setsockopt(IPV6_V6ONLY=%d)", v6only); if (setsockopt(sd, IPPROTO_IPV6, IPV6_V6ONLY, (void *) &v6only, sizeof(v6only))) { - msg(M_NONFATAL|M_ERRNO, "Setting IPV6_V6ONLY=%d failed", v6only); + msg(M_NONFATAL | M_SKERR, "Setting IPV6_V6ONLY=%d failed", v6only); } } if (bind(sd, cur->ai_addr, cur->ai_addrlen)) { - msg(M_FATAL | M_ERRNO, "%s: Socket bind failed on local address %s", + msg(M_FATAL | M_SKERR, "%s: Socket bind failed on local address %s", prefix, print_sockaddr_ex(local->ai_addr, ":", PS_SHOW_PORT, &gc)); } @@ -2254,7 +2254,7 @@ link_socket_close(struct link_socket *sock) msg(D_LOW, "TCP/UDP: Closing socket"); if (openvpn_close_socket(sock->sd)) { - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket failed"); + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket failed"); } } sock->sd = SOCKET_UNDEFINED; @@ -2271,7 +2271,7 @@ link_socket_close(struct link_socket *sock) { if (openvpn_close_socket(sock->ctrl_sd)) { - msg(M_WARN | M_ERRNO, "TCP/UDP: Close Socket (ctrl_sd) failed"); + msg(M_WARN | M_SKERR, "TCP/UDP: Close Socket (ctrl_sd) failed"); } sock->ctrl_sd = SOCKET_UNDEFINED; } @@ -3668,7 +3668,7 @@ sockethandle_finalize(sockethandle_t sh, /* if no error (i.e. just not finished yet), then DON'T execute this code */ io->iostate = IOSTATE_INITIAL; ASSERT(ResetEvent(io->overlapped.hEvent)); - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion error"); + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 I/O: Completion error"); } } break; @@ -3681,7 +3681,7 @@ sockethandle_finalize(sockethandle_t sh, /* error return for a non-queued operation */ SocketHandleSetLastError(sh, io->status); ret = -1; - msg(D_WIN32_IO | M_ERRNO, "WIN32 I/O: Completion non-queued error"); + msg(D_WIN32_IO | SocketHandleErrorFlag(sh), "WIN32 I/O: Completion non-queued error"); } else { diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index e9f1524d..a7ec827a 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -301,6 +301,12 @@ SocketHandleSetInvalError(sockethandle_t sh) sh.is_handle ? SetLastError(ERROR_INVALID_FUNCTION) : WSASetLastError(WSAEINVAL); } +static inline int +SocketHandleErrorFlag(sockethandle_t sh) +{ + return sh.is_handle ? M_ERRNO : M_SKERR; +} + #else /* ifdef _WIN32 */ #define openvpn_close_socket(s) close(s) diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c index 768bb613..10107c18 100644 --- a/src/openvpn/socks.c +++ b/src/openvpn/socks.c @@ -117,7 +117,7 @@ socks_username_password_auth(struct socks_proxy_info *p, if (size != strlen(to_send)) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port write failed on send()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port write failed on send()"); goto cleanup; } @@ -145,14 +145,14 @@ socks_username_password_auth(struct socks_proxy_info *p, /* timeout? */ if (status == 0) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read timeout expired"); + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read timeout expired"); goto cleanup; } /* error */ if (status < 0) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on select()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read failed on select()"); goto cleanup; } @@ -162,7 +162,7 @@ socks_username_password_auth(struct socks_proxy_info *p, /* error? */ if (size != 1) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on recv()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_username_password_auth: TCP port read failed on recv()"); goto cleanup; } @@ -204,7 +204,7 @@ socks_handshake(struct socks_proxy_info *p, size = send(sd, method_sel, sizeof(method_sel), MSG_NOSIGNAL); if (size != sizeof(method_sel)) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write failed on send()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port write failed on send()"); return false; } @@ -232,14 +232,14 @@ socks_handshake(struct socks_proxy_info *p, /* timeout? */ if (status == 0) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read timeout expired"); + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read timeout expired"); return false; } /* error */ if (status < 0) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read failed on select()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read failed on select()"); return false; } @@ -249,7 +249,7 @@ socks_handshake(struct socks_proxy_info *p, /* error? */ if (size != 1) { - msg(D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port read failed on recv()"); + msg(D_LINK_ERRORS | M_SKERR, "socks_handshake: TCP port read failed on recv()"); return false; } @@ -342,14 +342,14 @@ recv_socks_reply(socket_descriptor_t sd, /* timeout? */ if (status == 0) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read timeout expired"); + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read timeout expired"); return false; } /* error */ if (status < 0) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read failed on select()"); + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read failed on select()"); return false; } @@ -359,7 +359,7 @@ recv_socks_reply(socket_descriptor_t sd, /* error? */ if (size != 1) { - msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read failed on recv()"); + msg(D_LINK_ERRORS | M_SKERR, "recv_socks_reply: TCP port read failed on recv()"); return false; } @@ -484,7 +484,7 @@ establish_socks_proxy_passthru(struct socks_proxy_info *p, const ssize_t size = send(sd, buf, 5 + len + 2, MSG_NOSIGNAL); if ((int)size != 5 + (int)len + 2) { - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: TCP port write failed on send()"); + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: TCP port write failed on send()"); goto error; } } @@ -527,7 +527,7 @@ establish_socks_proxy_udpassoc(struct socks_proxy_info *p, 10, MSG_NOSIGNAL); if (size != 10) { - msg(D_LINK_ERRORS | M_ERRNO, "establish_socks_proxy_passthru: TCP port write failed on send()"); + msg(D_LINK_ERRORS | M_SKERR, "establish_socks_proxy_passthru: TCP port write failed on send()"); goto error; } }