Bugs item #1896698, was opened at 2008-02-19 08:16
Message generated for change (Comment added) made by bagder
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1896698&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: crash
>Status: Closed
Resolution: Fixed
Priority: 6
Private: No
Submitted By: Zmey (zmey_)
Assigned to: Daniel Stenberg (bagder)
Summary: NULL pointer dereferenced in Curl_done()

Initial Comment:
The problem appears both in libcurl 7.16.4 and 7.18.0 (latest to date).
Application crashes when traversing an FTP site (which is simply a series of FTP LIST commands). The crash is at the beginning of Curl_done() when it is called with connection pointer equal to NULL:

file url.c:
CURLcode Curl_done(struct connectdata **connp, /* 'connp' points to NULL */
                   CURLcode status,
                   bool premature)
{
  CURLcode result;
  struct connectdata *conn = *connp; /* assigning NULL to 'conn' */
  struct SessionHandle *data = conn->data; /* dereferencing... Oops! */

...

Curl_done() is called from Curl_perform():

file transfer.c:

CURLcode Curl_perform(struct SessionHandle *data)
{
...
  do {
    res = connect_host(data, &conn); /* primary connection */
    if(res == CURLE_OK) {
...
      /* !!! note: this may nullify the connection! */
      res = Curl_do(&conn, &do_done);

      if(res == CURLE_OK) {
...
/* long and ugly-looking sequence of nested if()'s */
...
      }
      else /* !!! note: Curl_do() returned failure, 'conn' may be NULL */
        /* Curl_do() failed, clean up left-overs in the done-call */
        res2 = Curl_done(&conn, res, FALSE); /* oops... */

Proposed solution:
Check for NULL connection at the beginning of Curl_done():

file url.c:

CURLcode Curl_done(struct connectdata **connp,
                   CURLcode status,
                   bool premature)
{
  CURLcode result;
  struct connectdata *conn = *connp;
  struct SessionHandle *data;

  if(conn == NULL)
     return CURLE_OK; /* or some error code */

  data = conn->data; /* safe to dereference */

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

>Comment By: Daniel Stenberg (bagder)
Date: 2008-02-21 18:54

Message:
Logged In: YES
user_id=1110
Originator: NO

Thanks a lot. Code committed to CVS just now. Case closed!

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

Comment By: Zmey (zmey_)
Date: 2008-02-21 16:03

Message:
Logged In: YES
user_id=1835005
Originator: YES

The fix looks logical for me. :)
You can credit me as Zmey Petroff <zmeypetroff_at_newmail.ru> if you wish.

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

Comment By: Daniel Stenberg (bagder)
Date: 2008-02-21 13:23

Message:
Logged In: YES
user_id=1110
Originator: NO

btw, if you give me your real name I'll make sure to credit you for this
finding in the changelog!

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

Comment By: Daniel Stenberg (bagder)
Date: 2008-02-21 13:18

Message:
Logged In: YES
user_id=1110
Originator: NO

Aha, so the conn pointer is NULL already when Curl_do() returns (an error
code).

So what about a fix like this:

--- lib/transfer.c 20 Feb 2008 08:28:02 -0000 1.383
+++ lib/transfer.c 21 Feb 2008 12:17:30 -0000
@@ -2389,7 +2389,7 @@
         if(CURLE_OK == res)
           res = res2;
       }
- else
+ else if(conn)
         /* Curl_do() failed, clean up left-overs in the done-call */
         res2 = Curl_done(&conn, res, FALSE);

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

Comment By: Zmey (zmey_)
Date: 2008-02-21 08:39

Message:
Logged In: YES
user_id=1835005
Originator: YES

I will try my best to elaborate on it, but I doubt that you will be able
to reproduce it.

First the globals.

The application recursively traverses an FTP server with lots of
subdirectories and terabytes of data. The server is not accessible from
Internet (it's in a local network), so you will not be able to connect to
it for tests. The application is quite complex and scripts-driven, so I
cannot post it here.

The error is quite rare. The application could run for hours without
crashing, but the amount of directories in the server is so huge that even
this rare bug appears regularly. Note that the server does not allow
multiple connection from the same IP address. Sometimes it refuses to
authenticate you after abnormal disconnection (it thinks you're still
logged in).

Now the details.

The application creates one easy curl handle. The options of the handle
are:

CURLOPT_MAXCONNECTS = 1 (this is important: I didn't notice any crashes
before, when the default was 5).
CURLOPT_NOSIGNAL = 1 (true)
CURLOPT_URL is in form "ftp://10.x.x.x/path/path1/" (ends in slash, so the
library retrieves the directory listing).
CURLOPT_TIMEOUT = 120 seconds (2 minutes)
CURLOPT_CONNECTTIMEOUT = 180 seconds (3 minutes)
CURLOPT_WRITEFUNCTION, CURLOPT_WRITEDATA - set.
CURLOPT_HEADERFUNCTION, CURLOPT_WRITEHEADER - also set.
CURLOPT_ERRORBUFFER - set to a buffer.

I have enabled verbose operation. I have also added some lines with
fprintf(stderr, "%s:%d - connection is %p\n", __FILE__, __LINE__, conn); -
to print traces in Curl_perform(). You can check the line numbers printed
in the trace (file 'transfer.c' in curl 7.18.0; note that each added line
shifted the text downwards):
.\transfer.c:2371 - between lines 2370 and 2371;
.\transfer.c:2379 - between lines 2377 and 2378;
.\transfer.c:2418 - between lines 2413 and 2414 (I had to add curly
brackets to make it a block).

So here is the trace:

Getting listing of "ftp://10.x.x.x/Soundtracks/J/Jurassic Park
III(2001)/"
* Re-using existing connection! (#0) with host 10.x.x.x
* Connected to 10.x.x.x (10.x.x.x) port 21 (#0)
.\transfer.c:2371 - connection is 0xab1bd8
> CWD /
< 250 CWD command successful
> CWD Soundtracks
< 250 CWD command successful
> CWD J
< 250 CWD command successful
> CWD Jurassic Park III(2001)
< 250 CWD command successful
> EPSV
* Connect data stream passively
< 229 Entering Extended Passive Mode (|||51060|)
* Trying 10.x.x.x... * Connection refused
* couldn't connect to host
* got positive EPSV response, but can't connect. Disabling EPSV
* Send failure: Connection was reset
* Re-used connection seems dead, get a new one
* Closing connection #0
* About to connect() to 10.x.x.x port 21 (#0)
* Trying 10.x.x.x... * connected
* Connected to 10.x.x.x (10.x.x.x) port 21 (#0)
< 220 FTP Server Ready ...
> USER anonymous
< 331 Anonymous login ok, send your complete email address as your
password.
> PASS ftp_at_example.com
< 530 Maximum number of connections from your IP-address exceeded.
* Access denied: 530
* Closing connection #0
.\transfer.c:2379 - connection is (nil)
.\transfer.c:2418 - connection is (nil)
(* crash in Curl_done() *)

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

Comment By: Daniel Stenberg (bagder)
Date: 2008-02-20 12:59

Message:
Logged In: YES
user_id=1110
Originator: NO

Can you please elaborate on exactly when this happens and how it comes it
passes NULL to this function? Can you provide a stand-alone example app
that can reproduce this problem?

NULL is not a legal value to pass in so I think we should work out why it
happened in the first place rather than just make Curl_done() deal with it.

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

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1896698&group_id=976
Received on 2008-02-21