Menu

#587 Curl_ourerrno implementation, GetLastError, WSAGetLastError

closed-fixed
libcurl (356)
5
2013-06-21
2006-05-03
Anonymous
No

Hi,

I've compiled and used libcurl version 7.15.3 on the
Windows platform and I think I found a bug.

It has to do with the Curl_ourerrno() function in
connect.c.

This function exists to abstract from platform
differences to make the code more portable.

For the windows platform, Curl_ourerrno() returns the
return value of the GetLastError() function, while for
other platforms it returns the value of the errno
variable.

However, on the windows platform, I think that
Curl_ourerrno() should return the return value of the
WSAGetLastError() function instead of the return value
of the GetLastError() function.

MSDN documentation states that GetLastError() will not
return error information about socket operations,
WSAGetLastError() should be called instead.

I've tested this and this is indeed the case. One
occurrence of the issue is in singleipconnect() in
connect.c:

if(-1 == rc) {

error = Curl_ourerrno();

switch (error) {
case EINPROGRESS:
case EWOULDBLOCK:
rc = waitconnect(sockfd, timeout_ms);
.
.

There is a piece of code here that in the case of an
error (-1 == rc), retrieves the error value using
Curl_ourerrno() and performs an action based on its
value (the switch statement).

In the case of a non-blocking socket, EWOULDBLOCK
might occur, but this is not fatal, the library just
should try again. The code in the EWOULDBLOCK case
does exactly that, but on the windows platform, the
Curl_ourerrno() function will never return
EWOULDBLOCK, because it is implemented using a
GetLastError() call instead of a WSAGetLastError()
call. I replaced GetLastError() with WSAGetLastError()
and then it does work.

I haven't explored other occurrences, but it might be
an issue in more places.

Can someone confirm if this is indeed a bug, and
whether it has been noticed before?

My proposed fix is to replace GetLastError() with
WSAGetLastError(), but I'm not sure whether
Curl_ourerrno() is also used to retrieve non-socket
related errors (in those cases GetLastError() should
be called).

I'm using Windows 2000 professional
version 5.0 (Build 2195: Service Pack 4),
but I assume the issue will occur on all windows
platforms.

With kind regards,

Roland Blom
Triple IT, Netherlands
curl@roland.triple-it.nl

Discussion

  • Daniel Stenberg

    Daniel Stenberg - 2006-05-04

    Logged In: YES
    user_id=1110

    I don't know much about windows, but it certainly sounds
    like you've idenfified a real problem here.

    I think I can just declare that Curl_ourerrno() must only be
    used to get socket-related errnos, as I believe those are
    the only cases where we want/need them.

    I guess there's no way to reliably first call GetLastError()
    and if no error is set call WSAGetLastError()?

     
  • Michele Bini

    Michele Bini - 2006-05-04

    Logged In: YES
    user_id=108343

    I'm not a windows guru but I did a bit of programming on the platform and
    I'm quite sure that as far as you use Curl_ourerrno() only after socket
    operations changing it to call WSAGetLastError() should work.

    A single function calling WSAGetLastError() and GetLastError() in a "smart" way
    cannot have the same behaviour as on unix because GetLastError() is not
    reset by winsock calls and, conversely, WSAGetLastError() is not reset by
    win32 calls.

    The usual solution to this problem is having two wrapper functions, lets say
    Curl_ourerrno() and Curl_our_sock_errno(). The latter calls WSAGetLastError()
    and is used only after socket operations; the former calls GetLastError() and is
    used anywhere else. On unix both simply check errno. Obviously if curl only
    checks for socket errors this is an overkill.

    In addition note that, although most win32/winsock calls reset GetLastError
    ()/WSAGetLastError() on success there is no guarantee that all of them do and
    thus, depending on the assumptions in the surrounding code, you may also
    need a call to SetLastError(0) or WSASetLastError(0) in Curl_ourerrno(). Strictly
    speaking to mimick unix behaviour you should do this immediately before a
    socket operation, but doing it in the error checking function tipically works.

     
  • Daniel Stenberg

    Daniel Stenberg - 2006-05-04
    • status: open --> closed-fixed
     
  • Daniel Stenberg

    Daniel Stenberg - 2006-05-04

    Logged In: YES
    user_id=1110

    Thanks a lot, code fixed and committed just now!

     
  • Nobody/Anonymous

    Logged In: NO

    Thank you!

    I tried the revised code and it works. You've implemented
    about the same fix that I made, you just gave the function
    a different name.

    Indeed, there is no reliable way to first call GetLastError
    () and only call WSAGetLastError() if GetLastError()
    doesn't return an error, because GetLastError() might
    contain an old error. Maybe it could be done if both error
    types were reset before calling the operation of which we
    want to check errors afterwards.

    Anyway, your fix is defninitely an improvement!

    Roland Blom
    Triple IT, Netherlands
    curl@roland.triple-it.nl