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 |
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
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 <<a href="mailto:lstipakov@gmail.com">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">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'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'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>
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 --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; }