Bug 18832 – [curl] file upload does not work

• Component: WebKitGTK
• Status: RESOLVED FIXED

[arno.]

Hi,
I'm compiled gtk-webkit with curl backend on a debian unstable.

[arno.]

Hi,
I'm compiled gtk-webkit with curl backend on a debian unstable, and file upload does not work.

In send a negative Content-Length header to the server. 
For example, "Content-Length: -5191020756686863821" (with a file of 369 bytes). Then, I usually get a message error from the server.

I think that's because libcurl is build with large file support (making sizeof(curl_off_t) equal to 8), while webkit is not (making sizeof(curl_off_t) equal to 4).

So, in libcurl (lib/http.c): 

#if SIZEOF_CURL_OFF_T > 4
#define FORMAT_OFF_T "lld" 
#else                      
#define FORMAT_OFF_T "ld"
#endif

[...]

   result = add_bufferf(req_buffer,
                           "Content-Length: %" FORMAT_OFF_T "\r\n",
                           http->postsize);

outputs a negative value.



In  WebCore/platform/network/curl/ResourceHandleManager.cpp, if I use, in function ResourceHandleManager::setupPOST:

curl_easy_setopt(d->m_handle, CURLOPT_POSTFIELDSIZE, size);

instead of 

 curl_easy_setopt(d->m_handle, CURLOPT_POSTFIELDSIZE_LARGE, size)

Content-Length header is correct, and file upload works.

I tried to add AC_SYS_LARGEFILE in configure.ac, but it seems to create aconfig.h which is not sourced by WebCore/platform/network/curl/ResourceHandleManager.cpp

For testing purpose, I have tried to put
#define _FILE_OFFSET_BITS 64 at the very top of WebCore/platform/network/curl/ResourceHandleManager.cpp and it fixes the bug.

[Marco Barisione]

Definining _FILE_OFFSET_BITS is not enough as CURL could be compiled without large files support even if it's available on your system, so you would get the opposite problem: passing a 64-bit integer when curl is expecting a 32-bit integer.

IMO this is a problem in the CURL API but we can workaround it checking at run-time for large file support and passing a different type based on the check.

I will write a patch in the next days.

[Julien Chaffraix]

> IMO this is a problem in the CURL API but we can workaround it checking at
> run-time for large file support and passing a different type based on the
> check.

I am not sure it is the right thing to do. What you want is to have the same size for curl_off_t in WebKit and in libcURL. It could be done by checking for LFS in libcURL at compile time and enable it in WebKit too (including aconfig.h if necessary). I have tried something along that path that I could post here if you want to try it.

As you said, it is an issue with libcurl but it is currently been tackled [1] and the next release should provide a specific header which should solve this problem and avoid a work-around.

[1] http://curl.haxx.se/lxr/source/TODO-RELEASE

[Marco Barisione]

(In reply to comment #3)
> I am not sure it is the right thing to do. What you want is to have the same
> size for curl_off_t in WebKit and in libcURL. It could be done by checking for
> LFS in libcURL at compile time and enable it in WebKit too (including aconfig.h
> if necessary). I have tried something along that path that I could post here if
> you want to try it.

What happens if your distro is using a curl library without large file support when you compile WebKit and that it switches to one with LFS?

aconfig.h should be included by config.h (see bug #20380) but for now we can include it directly if needed.

[Julien Chaffraix]

> What happens if your distro is using a curl library without large file support
> when you compile WebKit and that it switches to one with LFS?

Indeed, we may have this problem again. I was thinking of adding a runtime assertion to detect that kind of (corner?) case but it would not show up for release build and would have a hard time finding the cause. The runtime check for LFS seems to be the way to go for this use case.

However the issue here is that curl_off_t does not have the right size because WebKit did not enable LFS when libcURL did. This is why I proposed the compile time check (and later use the new header). Your runtime solution will not detect that as the problem is at compile time.

> aconfig.h should be included by config.h (see bug #20380) but for now we can
> include it directly if needed.

Nice idea.

[Marco Barisione]

Mike, what do you think about this?

[Michael Emmel]

(In reply to comment #6)
> Mike, what do you think about this?
> 

I think it should be handled in curl itself.

But the best fix is probably to static link the right version of curl instead of depending on the shared library. So at the minimum a static link option for curl should be added that should clear this up.

[Marco Barisione]

(In reply to comment #7)
> But the best fix is probably to static link the right version of curl instead
> of depending on the shared library. So at the minimum a static link option for
> curl should be added that should clear this up.

Distros would hate us for that. Why not a dynamic check as the solution for this and then also add AC_SYS_LARGEFILE so also other parts of WebKit GTK can benefit from this?

[Michael Emmel]

(In reply to comment #8)
> (In reply to comment #7)
> > But the best fix is probably to static link the right version of curl instead
> > of depending on the shared library. So at the minimum a static link option for
> > curl should be added that should clear this up.
> 
> Distros would hate us for that. Why not a dynamic check as the solution for
> this and then also add AC_SYS_LARGEFILE so also other parts of WebKit GTK can
> benefit from this?
> 

I just suggested it should be and option so we can opt to use the features of the latest version of curl before it gets into distros. Curl itself could use a bit of work to turn it into a good library for browsers static linking and co-development is a good way to move both projects forward. As and example content detection using a "file" command library is missing among other things.
This capability should be implemented in Curl IMHO.

They may hate us but realistically curl needs work and we need to move ahead of the distro for a while if you want to have a excellent connection library.

[Marco Barisione]

(In reply to comment #9)
> They may hate us but realistically curl needs work and we need to move ahead of
> the distro for a while if you want to have a excellent connection library.

More than hate us they would completely ignore the statically linked library. Most distros do that even when it means to lose functionalities or have bugs, so I really think that a workaround is better.

[Michael Emmel]

(In reply to comment #10)
> (In reply to comment #9)
> > They may hate us but realistically curl needs work and we need to move ahead of
> > the distro for a while if you want to have a excellent connection library.
> 
> More than hate us they would completely ignore the statically linked library.
> Most distros do that even when it means to lose functionalities or have bugs,
> so I really think that a workaround is better.
> 

No problem I'm just pointing out that curl/webkit integration has bigger issues that need to be resolved and a route that would allow both groups to work together until they are resolved. I'm not suggesting you not put the workaround in but I felt that this was a good place to insert my overall observation that curl needs refactoring to work well with browsers. Temporarily static linking in a development version of curl focused on browser support puts the project on the right track. Sorry to give a big picture answer its better I guess to keep hacking. I tried to take this approach with my ncurl patch and it looks like we are simply not going to build a good solution using curl.

And sorry for the rant but you need to be aware that curl is it stands now is not a good solution. Its got a number of serious problems that need to be fixed.

Rant off and I'll go back to my hidey hole.

[Marco Barisione]

Created attachment 23876 [details]
Pass an integer of the right size based on curl's compile options

[Alp Toker]

Comment on attachment 23876 [details]
Pass an integer of the right size based on curl's compile options

I agree this change is necessary since we have no control over which versions of libraries distributors will link against.

>Index: WebCore/platform/network/curl/ResourceHandleManager.cpp
>===================================================================
>--- WebCore/platform/network/curl/ResourceHandleManager.cpp	(revisione 36996)
>+++ WebCore/platform/network/curl/ResourceHandleManager.cpp	(copia locale)
>@@ -3,6 +3,7 @@
>  * Copyright (C) 2006 Michael Emmel [e-mail]
>  * Copyright (C) 2007 Alp Toker <[e-mail]>
>  * Copyright (C) 2007 Holger Hans Peter Freyther
>+ * Copyright (C) 2008 Collabora Ltd.
>  * All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -377,12 +378,24 @@
>     }
> 
>     // Obtain the total size of the POST
>+    // The size of a curl_off_t could be different in WebKit and in cURL depending on
>+    // compilation flags of both. For CURLOPT_POSTFIELDSIZE_LARGE we have to pass the
>+    // right size or random data will be used as the size.
>+    int expectedSizeOfCurlOffT = 0;
>+    if (!expectedSizeOfCurlOffT) {
>+        curl_version_info_data *infoData = curl_version_info(CURLVERSION_NOW);
>+        if (infoData->features & CURL_VERSION_LARGEFILE)
>+            expectedSizeOfCurlOffT = sizeof(long long);
>+        else
>+            expectedSizeOfCurlOffT = sizeof(int);
>+    }
>+

Looks like there's something wrong above, since !expectedSizeOfCurlOffT will always be true.

perhaps 'int expectedSizeOfCurlOffT' should be static?

I won't be able to test this one against different versions of curl myself but I'm guessing Marco has already tested it.

Looks good otherwise.

[Marco Barisione]

Created attachment 23904 [details]
Pass an integer of the right size based on curl's compile options

Yeah, I forgot a static.

I tested the patch with curl compiled with and without LFS and with WebKit compiled with and without LFS.

[Alp Toker]

Comment on attachment 23904 [details]
Pass an integer of the right size based on curl's compile options

r=me

[Jan Alonzo]

Landed in r37281