libimobiledevice / libimobiledevice-glue

A library with common code used by libraries and tools around the libimobiledevice project
GNU Lesser General Public License v2.1
86 stars 69 forks source link

Error code API mismatch on Win32 #14

Closed MarSoft closed 3 weeks ago

MarSoft commented 2 years ago

When building for Win32, socket code from the glue library gets error codes with WSAGetLastError() function rather than errno variable, which is correct: e.g. 1 When some error happens, we sometimes set errno ourselves: 1 2 3 In other cases, when we detect that error happened we don't touch errno and just return -1 to indicate error: 1 But the code in libusbmuxd always expects that errno will be set to some non-zero value if error happened: 1 2 3 As a result, on Windows some socket-related errors are not "masked" by libusbmuxd because errno is 0. This results to bugs later on, when the surrounding code thinks that no error have happened.

This should be fixed in either of the following approaches:

  1. Fix libusbmuxd (and probably other users of socket_* code) to not expect non-zero errno in case of error: if (res < 0 && errno != 0) res = -errno; etc
  2. Somehow "map" WSAGetLastError() codes to corresponding errno values in socket.c functions. This may be hard to achieve but would give the best results for the surrounding code which would then be able to properly report errors to the user.
  3. Just make sure that errno is non-zero if we are going to return error. For example, we can do errno = WSAGetLastError() to return WSA error code (which is always > 10000 in case of error), or errno = EIO to always return I/O error if any WSA error happened (this is ugly though), or maybe map some WSA errors to corresponding errno values and return others as is.

I think the last option would be the best one. We can map several known WSA error codes to errno values, and for other errors just set errno to the raw WSA code just to indicate that an error have happened. This will abstract platform-dependent error handling logic from the outer code, and still allow it to handle some known errors like EAGAIN. The only problem with this approach is that the outer code won't be able to properly describe an error to the user, because WSA error codes won't be handled by strerror - for example, here. This code is also where partial error mapping would be helpful - if we map WSAECONNREFUSED to ECONNREFUSED maybe some WSA code to ENODEV then idevice_connect will return the right errors.

sandin commented 2 years ago

Solution of nginx:

/os/win32/ngx_errno.h

#define ngx_socket_errno           WSAGetLastError()

u_char *ngx_strerror(ngx_err_t err, u_char *errstr, size_t size); // -> FormatMessage(err)

/os/unix/ngx_errno.h

#define ngx_socket_errno           errno

u_char *ngx_strerror(ngx_err_t err, u_char *errstr, size_t size);   // -> strerror(err);
mexmer commented 2 years ago

nginx solution looks good, there is more errors with windows socket, than just from send, also recv and select needs check for WSA there is also incosistency in error values, when it comes to socket function in glue, some return -1, and caller needs to resolve errno, while some return -errno (which of course doesn't work properly on windows). that needs to be put in line.

mexmer commented 2 years ago

when i think about all that, if we want to propagate errno further (Which seems to be the case, looking at libimobiledevice and libusbmuxd), probly safest aproach will be to wrap every used socket function inside some function in glue, then if return value of said function is SOCKET_ERROR (on WIN32 it's -1), then we can translate WSAGetLastError to errno, and set errno to said value, because on WIN32 errno will be 0 anyways ... then you can let rest of code use errno and strerror.

other option will be to never read either errno or strerror directly, but make wrappers around those two, and handle them accordingly (like nginx does), but seems too much pain to me.

nikias commented 4 months ago

@MarSoft I came up with this fc10c88395f3fefd3b30f3ef9354c45cef7136a6