Bugs item #1759542, was opened at 2007-07-24 14:53
Message generated for change (Comment added) made by bagder
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1759542&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: bad behaviour
Status: Open
Resolution: None
>Priority: 7
Private: No
Submitted By: Jayesh A Shah (jayesh_a_shah)
Assigned to: Daniel Stenberg (bagder)
Summary: curl writing to a wrong file descriptor causing corruption

Initial Comment:
Hi,

We are facing a issue with curl library in secure mode.

I have written down a simple program to demonstrate the issue with the curl /openssl library. This issue is seen even with latest curl library (7.16.4) and ssl (0.9.8e) libraries.

In this sample program, I do the following:

1. Initialize curl and ssl parameters
2. create a get thread
3. create a apply thread
4. wait for threads

get thread:
 
1. In the get thread, I get one file from a remote server and then notify the apply thread using global variable. To be specific, I am downloading file abc.dat to local directory as xyz.dat

In our real application, we make use of ace instead of pthreads and Ace_Message_Queue for notifying apply thread.

Apply thread:

 Wait for get thread to finish getting the data.
  open the file /root/test.dat.
  sleep for 10 secs
  close the fd for /root/test.dat

To compile this program:
  g++ -g -I<curl include path> main.cpp -lpthread -L<curl lib dir> -lcurl -L<ssl lib dir> -lssl -lcrypto -ldl -lz -o my_curl

To run this program in non secure mode:

./my_curl

To run this program in secuee mode:

./my_curl secure

When I run the program in non secure mode, there are no issues. When I run it in non secure mode, the file /root/test.dat gets corrupted.

-------------------------------------------------------Non Secure Mode:

[root_at_IMITS027 host]# ll /root/test.dat
-rw-r--r-- 1 root root 0 Jul 24 15:15 /root/test.dat
[root_at_IMITS027 host]# rm xyz.dat
rm: remove regular file `xyz.dat'? y
[root_at_IMITS027 host]# ./my_curl
Assuming non secure mode
 [root_at_IMITS027 host]# ll /root/test.dat
-rw-r--r-- 1 root root 0 Jul 24 15:15 /root/test.dat
[root_at_IMITS027 host]# ll xyz.dat
-rw-r--r-- 1 root root 595661 Jul 24 15:15 xyz.dat

Secure Mode:

[root_at_IMITS027 host]# rm xyz.dat
rm: remove regular file `xyz.dat'? y
[root_at_IMITS027 host]# rm /root/test.dat
[root_at_IMITS027 host]# touch /root/test.dat
[root_at_IMITS027 host]# ll /root/test.dat
-rw-r--r-- 1 root root 0 Jul 24 15:15 /root/test.dat
[root_at_IMITS027 host]# ./my_curl secure
running in secure mode
[root_at_IMITS027 host]# ll /root/test.dat
-rw-r--r-- 1 root root 37 Jul 24 15:16 /root/test.dat

Program code:
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
// File : main.cpp
//
// Description:
//

#include <pthread.h>
#include <openssl/crypto.h>
#include <curl/curl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#include <string.h>

struct FtpFile {
  char *filename;
  FILE *stream;
};

int my_fwrite(void *buffer, size_t size, size_t nmemb, void *stream)
{
  struct FtpFile *out=(struct FtpFile *)stream;
  if(out && !out->stream) {
    /* open file for writing */
    out->stream=fopen(out->filename, "wb");
    if(!out->stream)
      return -1; /* failure, can't open file to write */
  }
  return fwrite(buffer, size, nmemb, out->stream);
}

bool secure = false;
volatile bool getdone = false;

#define MUTEX_TYPE pthread_mutex_t
#define MUTEX_SETUP(x) pthread_mutex_init(&(x), NULL)
#define MUTEX_CLEANUP(x) pthread_mutex_destroy(&(x))
#define MUTEX_LOCK(x) pthread_mutex_lock(&(x))
#define MUTEX_UNLOCK(x) pthread_mutex_unlock(&(x))
#define THREAD_ID pthread_self( )

void handle_error(const char *file, int lineno, const char *msg){
     fprintf(stderr, "** %s:%i %s\n", file, lineno, msg);
     /* exit(-1); */
 }

/* This array will store all of the mutexes available to OpenSSL. */
static MUTEX_TYPE *mutex_buf= NULL;

static void locking_function(int mode, int n, const char * file, int line)
{
  if (mode & CRYPTO_LOCK)
    MUTEX_LOCK(mutex_buf[n]);
  else
    MUTEX_UNLOCK(mutex_buf[n]);
}

static unsigned long id_function(void)
{
  return ((unsigned long)THREAD_ID);
}

int thread_setup(void)
{
  int i;

  curl_global_init(CURL_GLOBAL_ALL);
  mutex_buf = (MUTEX_TYPE *)malloc(CRYPTO_num_locks( ) * sizeof(MUTEX_TYPE));
  if (!mutex_buf)
    return 0;
  for (i = 0; i < CRYPTO_num_locks( ); i++)
    MUTEX_SETUP(mutex_buf[i]);
  CRYPTO_set_id_callback(id_function);
  CRYPTO_set_locking_callback(locking_function);
  return 1;
}

int thread_cleanup(void)
{
  int i;

  curl_global_cleanup();
  if (!mutex_buf)
    return 0;
  CRYPTO_set_id_callback(NULL);
  CRYPTO_set_locking_callback(NULL);
  for (i = 0; i < CRYPTO_num_locks( ); i++)
    MUTEX_CLEANUP(mutex_buf[i]);
  free(mutex_buf);
  mutex_buf = NULL;
  return 1;
}

void * applydata(void * in)
{
        while(!getdone);
        int fd = open("/root/test.dat", O_RDWR);
        sleep(10);
        close(fd);
        return 0;
}

void * getdata(void * in)
{

    CURL *curl;
    CURLcode res;
    struct FtpFile ftpfile={
        "xyz.dat", /* name to store the file as if succesful */
        NULL
   };

   curl = curl_easy_init();
   curl_easy_setopt(curl, CURLOPT_USERPWD, "jayesh:passwd") ;
   curl_easy_setopt(curl, CURLOPT_PORT, 0) ;
   curl_easy_setopt(curl,CURLOPT_URL, "ftp://10.0.1.27//home/jayesh/abc.dat");
 
   if(secure)
   {
    // Setting SSL for both the data and control channels
    curl_easy_setopt(curl, CURLOPT_FTP_SSL, CURLFTPSSL_ALL);

    curl_easy_setopt(curl, CURLOPT_FTPSSLAUTH, CURLFTPAUTH_DEFAULT);

    //set the ssl version
    curl_easy_setopt(curl,CURLOPT_SSLVERSION,CURL_SSLVERSION_SSLv3);

    // Set up using the default engine
    curl_easy_setopt(curl, CURLOPT_SSLENGINE_DEFAULT, 1);

    // Don't perform peer or host verification
    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
    curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);

    curl_easy_setopt(curl, CURLOPT_SSLKEY, "passwd2");
    }

    /* Define our callback to get called when there's data to be written */
    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, my_fwrite);
    /* Set a pointer to our struct to pass to the callback */
    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &ftpfile);

    /* Switch on full protocol/debug output ?*/
    curl_easy_setopt(curl, CURLOPT_VERBOSE, true);

    res = curl_easy_perform(curl);
    if(CURLE_OK != res)
    {
      /* we failed */
      fprintf(stderr, "curl told us %d\n", res);
    }
        
    // this is only for test, we would send postmsg
    // only on success in out app
    getdone = true;

    sleep(1);
    /* always cleanup */
    curl_easy_cleanup(curl);

   if(ftpfile.stream)
    fclose(ftpfile.stream); /* close the local file */

   return 0;
}

int main(int argc, char ** argv)
{

   if(argc != 2 )
   {
        printf("Assuming non secure mode\n");
        secure = false;
   } else
   {
     if(!strcmp(argv[1], "secure"))
     {
        printf("running in secure mode\n");
        secure = true;
     } else
     {
        printf("running in non secure mode\n");
        secure = false;
     }
   }

    thread_setup();
    /* create two threads with message queue */

    pthread_t tid[2];
    pthread_create(&tid[0], NULL, getdata, NULL);
    pthread_create(&tid[1], NULL, applydata, NULL);
   

  /* now wait for all threads to terminate */
  for(int i=0; i< 2; i++) {
    pthread_join(tid[i], NULL);
    fprintf(stderr, "Thread %d terminated\n", i);
  }

  thread_cleanup();

  return 0;
}

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

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

>Comment By: Daniel Stenberg (bagder)
Date: 2007-07-25 23:50

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

Thanks for the description and yes, I can see the problem now.

Wouldn't the correct fix be to simply shutdown the SSL on the secondary
connection (only) before the close is done in Curl_ftp_done() ? (Of course
given that the second connection uses SSL)

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

Comment By: Jayesh A Shah (jayesh_a_shah)
Date: 2007-07-25 23:24

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

I should have mentioned that Curl_ossl_shutdown was initial tried instead
of creating the Curl_ossl_close_handle. However it caused hangs if multiple
curl_easy_performs were down with out calling curl_easy_cleanup.

Also another note on threads. This issue is reproducible with out the use
of threads. All you need to do is in getdata after the call to
curl_easy_perform and before the call to curl_easy_cleanup do an open of
test.dat (just like the applydata function) but don't close the fd until
after the call to curl_easy_cleanup. You could then in main just call
getdata directly with no threads and you will see that test.dat gets the
socket writes from the curl_easy_cleanup written to it.

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

Comment By: Jayesh A Shah (jayesh_a_shah)
Date: 2007-07-25 22:48

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

I'm a colleague of Jayesh's and he asked me to look into this for him.

To answer you questions this sample code fails on various Linux distros
for RedHat and Suse. As for threads, the issue is only related to threads
in the sense that to reproduce this issue requires the curl_easy_perform to
happen in a separate thread from the one where the test.dat is being open.
And the test.dat needs to be opened after the curl_easy_perform completes
but before the curl_easy_cleanup is run. So thread safe C libraries should
not come into play for this specific issue.

To be clear, this is only an issue when using secure mode (i.e. SSL). I
looked at version 7.16.4 and the issue (at least for FTP) is that in ftp.c
in the function Curl_ftp_done a call is made to sclose on the data socket
(at or near line 3118). The problem is that in secure mode, this closes the
socket but not the SSL part. So later when curl_easy_cleanup is called a
call is made to Curl_ossl_close (in ssluse.c). In there a call is made to
SSL_shutdown which eventually tries to write some data to the socket. In
this case the fd for that socket is no longer valid as it was closed in
FTP_done. In particular the fd has been reissued by the os and is now for
the file test.dat. That is where the data for the SSL write ends up. I
believe in general SSL needs to do some stuff before calling sclose on the
socket (it has been a long time since I have worked with SSL so I don't
remember exactly what specifically needs to happen with respect to SSL
prior to the sclose). Once the socket is closed then no more writes should
be attempted to that socket.

With that in mind, I created a temp fix. What was done was to break up the
Curl_ossl_close into 2 functions. Curl_ossl_close_handle and
Curl_oss_close. The code that does the SSL shutdown and set the connect
state from the original Curl_ossl_close was moved into
Curl_ossl_close_handle. Curl_ossl_close now calls Curl_ossl_close_handle
and continues to do what it was doing. In ftp.c before the sclose a call
was added to Curl_ossl_close_handle for the ssl handle that is associated
with the socket that is going to be closed. This is probably not complete
(as there maybe other places when SSL is being used where this needs to be
done before the sclose call) and perhaps not even the correct solution (I
am not at all familiar with the curl code, so the new
Curl_ossl_close_handle maybe doing more then it needs to or maybe not
enough), but it does seem eliminate the corruption (i.e. having SSL write
to the test.dat fd).

If I find anything else out I will let you know.

Anyway here are the diffs for you to see exactly what was done

diff ssluse.c new-ssluse.c
703a704,716
> * This function is called when an SSL handle is closed.
> */
> void Curl_ossl_close_handle( struct ssl_connect_data * connssl)
> {
> if(connssl->use) {
> if(connssl->handle) {
> (void)SSL_shutdown(connssl->handle);
> SSL_set_connect_state(connssl->handle);
> }
> }
> }
>
> /*
708,732c721,747
< int i;
< /*
< ERR_remove_state() frees the error queue associated with
< thread pid. If pid == 0, the current thread will have its
< error queue removed.
<
< Since error queue data structures are allocated
< automatically for new threads, they must be freed when
< threads are terminated in oder to avoid memory leaks.
< */
< ERR_remove_state(0);
<
< for(i=0; i<2; i++) {
< struct ssl_connect_data *connssl = &conn->ssl[i];
<
< if(connssl->handle) {
< (void)SSL_shutdown(connssl->handle);
< SSL_set_connect_state(connssl->handle);
<
< SSL_free (connssl->handle);
< connssl->handle = NULL;
< }
< if(connssl->ctx) {
< SSL_CTX_free (connssl->ctx);
< connssl->ctx = NULL;

---
>     struct ssl_connect_data * connssl;
>     if(conn->ssl[FIRSTSOCKET].use) {
>         int i;
>         /*
>           ERR_remove_state() frees the error queue associated with
>           thread pid.  If pid == 0, the current thread will have its
>           error queue removed.
> 
>           Since error queue data structures are allocated
>           automatically for new threads, they must be freed when
>           threads are terminated in oder to avoid memory leaks.
>         */
>         ERR_remove_state(0);
> 
>         for(i=0; i<2; i++) {
>             connssl = &conn->ssl[i];
>             if (connssl->handle) {          
>                 Curl_ossl_close_handle(connssl);
>                 SSL_free (connssl->handle);
>                 connssl->handle = NULL;
>             }
>         }
>         if(connssl->ctx) {
>             SSL_CTX_free (connssl->ctx);
>             connssl->ctx = NULL;
>         }
>         connssl->use = FALSE; /* get back to ordinary socket usage */
734,735d748
<     connssl->use = FALSE; /* get back to ordinary socket usage */
<   }
diff ssluse.h new-ssluse.h
34a35
> void Curl_ossl_close_handle( struct ssl_connect_data * connssl); /*
close a SSL handle */
diff ftp.c new-ftp.c
3117c3117,3118
< 
---
>   
>   Curl_ossl_close_handle(&conn->ssl[SECONDARYSOCKET]);
----------------------------------------------------------------------
Comment By: Dan Fandrich (dfandrich)
Date: 2007-07-24 18:29
Message:
Logged In: YES 
user_id=236775
Originator: NO
The test.dat file descriptor doesn't even exist until after the main curl
transfer runs; only curl_easy_cleanup() runs while the file is open, so if
it's a curl problem, that's where it would be. What are those 37 bytes that
test.dat contains at the end? Is the xyz.dat file transferred correctly?
Can you post the libcurl debug logs during this transfer (especially the
part during the curl_easy_cleanup)?
What platform is this, anyway? Are you using thread safe C libraries? 
----------------------------------------------------------------------
Comment By: Daniel Stenberg (bagder)
Date: 2007-07-24 16:23
Message:
Logged In: YES 
user_id=1110
Originator: NO
(please don't change the priority field)
I don't understand what the problem is. Writing to "wrong file
descriptor"? What does that mean exactly?
The writing to the file isn't even done by libcurl, it is done by your
provided callback!
----------------------------------------------------------------------
Comment By: Jayesh A Shah (jayesh_a_shah)
Date: 2007-07-24 15:33
Message:
Logged In: YES 
user_id=1852319
Originator: YES
Please note : There was a small typo in my submission ...
I had written:
---------------------------------------------------------------------------
When I run the program in non secure mode, there are no issues. When I
run
it in non secure mode, the file /root/test.dat gets corrupted.
----------------------------------------------------------------------------
Read it as :
When I run the program in non secure mode, there are no issues. When I
run
it in secure mode, the file /root/test.dat gets corrupted.
----------------------------------------------------------------------
Comment By: Jayesh A Shah (jayesh_a_shah)
Date: 2007-07-24 15:33
Message:
Logged In: YES 
user_id=1852319
Originator: YES
Please note : There was a small typo in my submission ...
I had written:
---------------------------------------------------------------------------
When I run the program in non secure mode, there are no issues. When I
run
it in non secure mode, the file /root/test.dat gets corrupted.
----------------------------------------------------------------------------
Read it as :
When I run the program in non secure mode, there are no issues. When I
run
it in secure mode, the file /root/test.dat gets corrupted.
----------------------------------------------------------------------
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=1759542&group_id=976