Bugs item #1481217, was opened at 2006-05-03 09:11
Message generated for change (Comment added) made by nobody
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1481217&group_id=976

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: libcurl
Group: portability problem
Status: Closed
Resolution: Fixed
Priority: 5
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Daniel Stenberg (bagder)
Summary: Curl_ourerrno implementation, GetLastError, WSAGetLastError

Initial Comment:

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_at_roland.triple-it.nl

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2006-05-05 06:51

Message:
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_at_roland.triple-it.nl

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2006-05-04 15:40

Message:
Logged In: YES
user_id=1110

Thanks a lot, code fixed and committed just now!

----------------------------------------------------------------------

Comment By: Michele Bini (mbini)
Date: 2006-05-04 01:33

Message:
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.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2006-05-03 23:09

Message:
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()?

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1481217&group_id=976
Received on 2006-05-05