curl_easy_perform locks when using mbedtls #737

Closed
damv opened this Issue Mar 29, 2016 · 14 comments

Projects

None yet

3 participants

@damv
damv commented Mar 29, 2016

I got an issue with libcurl after migrating from cyassl to mbedtls.

I configure my easy_handle to do a HTTPS POST than I call curl_easy_perform which locks and never returns. The same code works like a charm when using cyassl.
More precisely, with cyassl I can see that my write function (CURLOPT_WRITEFUNCTION) is called 2 times, one time for 212 bytes, and then for 9 bytes.
With mbedtls I receive the first call only. Sometimes the second call arrives after a long delay (several minutes).
Maybe it is the same bug than #334 ?
I use libcurl 7.47.1

@bagder bagder self-assigned this Mar 29, 2016
@bagder
Member
bagder commented Mar 29, 2016

Could be the same. Can you tell us how to reproduce this issue, ideally against a public URL?

@damv
damv commented Mar 30, 2016

Here is an example which triggers the problem.
For some reason, in this example easy_perform always returns, with a smaller delay (approximately 15 seconds).
I chose the size of the web page in order to have at least two calls to my write function. If the page is small enough, the write function is called only once, and there is no issue.

For information, the platform is a cortex M4 chip and the ciphersuite used is TLS-RSA-WITH-AES-128-GCM-SHA256. I will perform test with other ciphersuites.

Let me know if you are able to reproduce.

#define SIZE_SMLBUF       212
#define SIZE_LRGBUF       1800

int test_dvi ()
{
    char resbuf[SIZE_LRGBUF];
    char param[SIZE_SMLBUF];
    char *p
    printf("max_write_size %d\n", CURL_MAX_WRITE_SIZE);

    size_t writefunction(void *ptr, size_t size, size_t nmemb, void *stream) {
        if (p == NULL)
            return (size * nmemb);
        if ((p + size * nmemb) > resbuf + SIZE_LRGBUF + 100) {
            printf("[buff ovflow (%d>%d)\n", (int)((p - resbuf) + size * nmemb), (int)SIZE_LRGBUF + 100);
            resbuf[0] = 0;
            return (-1);
        }
        memcpy(p, ptr, size * nmemb);
        p += size * nmemb;
        p[0] = 0;
        printf("End of write, size = %d\n", size*nmemb);
        return (size * nmemb);
    }

    p = resbuf;
    if (resbuf != NULL) {
        p[0] = 0;
    }

    CURL *curl = curl_easy_init();
    if(curl) {
        curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
        curl_easy_setopt(curl, CURLOPT_URL, "https://178.62.74.69/");
        curl_easy_setopt(curl, CURLOPT_POST, 1);
        curl_easy_setopt(curl, CURLOPT_READFUNCTION, NULL);
        curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &writefunction);
        curl_easy_setopt(curl, CURLOPT_POSTFIELDS, param);
        curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, strlen(param));
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
        curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
        printf("easy perform...\n");
        curl_easy_perform(curl);
        printf("Result %s\n", resbuf);
        printf("clean up...\n");
        curl_easy_cleanup(curl);
    }

    return 0;
}
@bagder
Member
bagder commented Apr 2, 2016

I tried to reproduce it from my linux box, but failed:

$ ./src/curl -V
curl 7.49.0-DEV (x86_64-pc-linux-gnu) libcurl/7.49.0-DEV mbedTLS/2.2.1 zlib/1.2.8 c-ares/1.11.0 libidn/1.32 libpsl/0.11.0 (+libicu/55.1) libssh2/1.5.0 nghttp2/1.9.1 librtmp/2.3
$ time ./src/curl --data-binary @libtool https://178.62.74.69/ -k > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  332k  100   711  100  332k   1531   715k --:--:-- --:--:-- --:--:--  714k

real    0m0.474s
user    0m0.048s
sys     0m0.000s
$ echo $?
0
@bagder
Member
bagder commented Apr 2, 2016

Can you use a debugger or strace or something to figure out which function that gets stuck, or where the loop is?

@damv
damv commented Apr 6, 2016

I have finally found the bug.
To know if there is still data to be retrieved from mbedtls, the "function" called is :

#define curlssl_data_pending(x,y) (x=x, y=y, 0)

... which always returns 0.

I got the good behaviour with :

#define curlssl_data_pending(x,y) mbedtls_data_pending(x,y)
int mbedtls_data_pending(struct connectdata *conn, int sockindex)
{
    return mbedtls_ssl_pending(&conn->ssl[sockindex].ssl);
}

This problem will only occur with small values of CURL_MAX_WRITE_SIZE (=500 in my embedded application). Indeed, if CURL_MAX_WRITE_SIZE is sufficiently high, the expected return value will always be 0 and the trivial implementation works.

@bagder
Member
bagder commented Apr 6, 2016

@damv, Can you please produce a full patch or pull-request with the change you made to get it working?

@jay
Member
jay commented Apr 6, 2016

mbedtls_ssl_pending? where is that? I can't find it in the API.

@damv
damv commented Apr 7, 2016

My mistake, I had to define it myself, in mbedTLS :

int mbedtls_ssl_pending( mbedtls_ssl_context *ssl)
{
    return ssl->in_msglen != 0;
}

Without modifying mbedTLS we could define mbedtls_data_pending as :

int mbedtls_data_pending(struct connectdata *conn, int sockindex)
{
    mbedtls_ssl_context *ssl = &conn->ssl[sockindex].ssl;
    return ssl->in_msglen != 0;
}
@bagder
Member
bagder commented Apr 7, 2016

I don't think our own functions should use mbedtls_ as prefix. I know we have that already established but we should stop that so that it becomes easier to see the actual mbedtls functions.

If the function is to be provided to other source files within libcurl it needs to be prefixed with Curl_.

@bagder
Member
bagder commented Apr 7, 2016

Fixed my first point in 5446549

@bagder
Member
bagder commented Apr 7, 2016

So if I understand you correctly, the patch would be something like this?

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 77ff326..4f887a1 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -862,8 +862,13 @@ int Curl_mbedtls_init(void)
 void Curl_mbedtls_cleanup(void)
 {
   (void)Curl_polarsslthreadlock_thread_cleanup();
 }

-
+int Curl_mbedtls_data_pending(const struct connectdata *conn, int sockindex)
+{
+  mbedtls_ssl_context *ssl =
+    (mbedtls_ssl_context *)&conn->ssl[sockindex].ssl;
+  return ssl->in_msglen != 0;
+}

 #endif /* USE_MBEDTLS */
diff --git a/lib/vtls/mbedtls.h b/lib/vtls/mbedtls.h
index eb6192e..bd1c15b 100644
--- a/lib/vtls/mbedtls.h
+++ b/lib/vtls/mbedtls.h
@@ -29,10 +29,11 @@
 #include <mbedtls/sha256.h>

 /* Called on first use mbedTLS, setup threading if supported */
 int  Curl_mbedtls_init(void);
 void Curl_mbedtls_cleanup(void);
+int Curl_mbedtls_data_pending(const struct connectdata *conn, int sockindex);

 CURLcode Curl_mbedtls_connect(struct connectdata *conn, int sockindex);

 CURLcode Curl_mbedtls_connect_nonblocking(struct connectdata *conn,
                                            int sockindex,
@@ -61,11 +62,11 @@ int Curl_mbedtls_shutdown(struct connectdata *conn, int sockindex);
 #define curlssl_set_engine(x,y) (x=x, y=y, CURLE_NOT_BUILT_IN)
 #define curlssl_set_engine_default(x) (x=x, CURLE_NOT_BUILT_IN)
 #define curlssl_engines_list(x) (x=x, (struct curl_slist *)NULL)
 #define curlssl_version Curl_mbedtls_version
 #define curlssl_check_cxn(x) (x=x, -1)
-#define curlssl_data_pending(x,y) (x=x, y=y, 0)
+#define curlssl_data_pending(x,y) Curl_mbedtls_data_pending(x, y)
 #define CURL_SSL_BACKEND CURLSSLBACKEND_MBEDTLS
 #define curlssl_sha256sum(a,b,c,d) mbedtls_sha256(a,b,c,0)

 /* This might cause libcurl to use a weeker random!
    TODO: implement proper use of Polarssl's CTR-DRBG or HMAC-DRBG and use that
@damv
damv commented Apr 7, 2016

yes, exactly.

@damv
damv commented Apr 7, 2016

It is not related but I also had to do this patch to use mbedtls debug

From 29f05bacec716371049d636a14e31be999f50305 Mon Sep 17 00:00:00 2001
From: Damien Vielpeau <damien.vielpeau@withings.com>
Date: Thu, 7 Apr 2016 15:58:11 +0200
Subject: [PATCH] Fix mbedTLS debug

---
 lib/vtls/mbedtls.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 77ff326..9e61b07 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -101,7 +101,7 @@ static int entropy_func_mutex(void *data, unsigned char *output, size_t len)
 #undef MBEDTLS_DEBUG

 #ifdef MBEDTLS_DEBUG
-static void mbed_debug(void *context, int level, const char *line)
+static void mbed_debug(void *context, int level, const char *f_name, int line_nb, const char *line)
 {
   struct SessionHandle *data = NULL;

@@ -423,7 +423,7 @@ mbed_connect_step1(struct connectdata *conn,
 #endif

 #ifdef MBEDTLS_DEBUG
-  mbedtls_ssl_conf_dbg(&connssl->ssl, mbedtls_debug, data);
+  mbedtls_ssl_conf_dbg(&connssl->config, mbedtls_debug, data);
 #endif

   connssl->connecting_state = ssl_connect_2;
-- 
2.5.0
@bagder bagder added a commit that closed this issue Apr 7, 2016
@bagder bagder mbedtls: implement and provide *_data_pending()
... as otherwise we might get stuck thinking there's no more data to
handle.

Reported-by: Damien Vielpeau

Fixes #737
c111178
@bagder bagder closed this in c111178 Apr 7, 2016
@bagder
Member
bagder commented Apr 7, 2016

Thanks, I've pushed these commits now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment