/curl

Can not send 475020 byte file over chunked transfer encoding. #2001

Closed
moohoorama opened this Issue Oct 20, 2017 · 6 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
3 participants
@moohoorama @jay @bagder
@moohoorama

moohoorama commented Oct 20, 2017

I did this

Whenever sending 475020 byte file over chunked transfer encoding, a curl doesn't send last contents chunk. As far as I know, the curl sends 29 chunks of 16372 byte, and sends last contents chunk of 232 byte, finally, sends a zero chunk, which means "transferred all contents". But a curl doesn't send last contents chunk of 232 byte. So, a HTTP Servers wait to recv a zero chunk, Both client and server waits for each other, This request fail.
475018, 475019, 475021, 475022, All okay.

ex)

  1. 475020 byte
  • Client
$ truncate -s 475020 test.file
$ curl    -H "transfer-encoding:chunked" http://localhost:44444/ -T test.file -vvv -H 'Expect: '
* Hostname was NOT found in DNS cache
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 44444 (#0)
> PUT /test.file HTTP/1.1
> User-Agent: curl/7.35.0
> Host: localhost:44444
> Accept: */*
> transfer-encoding:chunked
> 
* We are completely uploaded and fine
  • Server
PUT /test.file HTTP/1.1
User-Agent: curl/7.35.0
Host: localhost:44444
Accept: */*
transfer-encoding:chunked


3ff4
XXXXXXXX
..
..
..
3ff4
XXXXXXXX
  1. 475021 byte
  • Client
$ truncate -s 475021 test.file
curl    -H "transfer-encoding:chunked" http://localhost:44444/ -T test.file -vvv -H 'Expect: '
...
> Accept: */*
> transfer-encoding:chunked
*
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Server: BaseHTTP/0.3 Python/2.7.6
< Date: Fri, 20 Oct 2017 05:47:18 GMT
* we are done reading and this is set to close, stop send
* Closing connection 0
  • Server
PUT /test.file HTTP/1.1
...

3ff4
XXXXXXXX
..
..
..
3ff4
XXXXX...
e9
XXXXX...

0
  1. 475019 byte
  • Client
$ truncate -s 475021 test.file
curl    -H "transfer-encoding:chunked" http://localhost:44444/ -T test.file -vvv -H 'Expect: '
...
> Accept: */*
> transfer-encoding:chunked
*
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Server: BaseHTTP/0.3 Python/2.7.6
< Date: Fri, 20 Oct 2017 05:47:18 GMT
* we are done reading and this is set to close, stop send
* Closing connection 0
  • Server
PUT /test.file HTTP/1.1
...

3ff4
XXXXXXXX
..
..
..
3ff4
XXXXX...
e7
XXXXX...

0

I expected the following

A curl send last 232 bytes chunk, and zero chunk.

curl/libcurl version

curl 7.35.0 (x86_64-pc-linux-gnu) libcurl/7.35.0 OpenSSL/1.0.1f zlib/1.2.8 libidn/1.28 librtmp/2.3

operating system

Ubuntu 14.04.5 LTS
Kernel 3.16.0-77-generic #99~14.04.1-Ubuntu SMP Tue Jun 28 19:17:10 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 20, 2017

Member

I can reproduce this in the latest curl. There is code that does not seem to account for chunked encoding, for example in the fail case it detects that the file has finished uploading when it hasn't:

curl/lib/transfer.c

Lines 1047 to 1053 in 3ea7679

k->writebytecount += bytes_written;
if(k->writebytecount == data->state.infilesize) {
/* we have sent all data we were supposed to */
k->upload_done = TRUE;
infof(data, "We are completely uploaded and fine\n");
}

Yet in the success case (I used 475021) that's never hit (shouldn't be anyway in my opinion), it just keeps uploading:

bytes: 16384
fillcount: 16380
k->writebytecount = k->writebytecount + bytes_written (442260 + 16380)
k->writebytecount: 458640
bytes: 16384
fillcount: 16380
k->writebytecount = k->writebytecount + bytes_written (458640 + 16380)
k->writebytecount: 475020
bytes: 16384
fillcount: 239
k->writebytecount = k->writebytecount + bytes_written (475020 + 239)
k->writebytecount: 475259
bytes: 16384
fillcount: 5
k->writebytecount = k->writebytecount + bytes_written (475259 + 5)
k->writebytecount: 475264

bytes is what's going in to fillreadbuffer and fillcount is what comes out. it just happens to work because k->writebytecount is never 475021 so it never caps it

so we could check chunked there and ignore infilesize in that case, or keep track of the whole thing? but if it's chunked we may not know the filesize. fillreadbuffer has some logic to deal with chunks

curl/lib/transfer.c

Lines 130 to 134 in 3ea7679

if(data->req.upload_chunky) {
/* if chunked Transfer-Encoding */
buffersize -= (8 + 2 + 2); /* 32bit hex + CRLF + CRLF */
data->req.upload_fromhere += (8 + 2); /* 32bit hex + CRLF */
}

Member

jay commented Oct 20, 2017

I can reproduce this in the latest curl. There is code that does not seem to account for chunked encoding, for example in the fail case it detects that the file has finished uploading when it hasn't:

curl/lib/transfer.c

Lines 1047 to 1053 in 3ea7679

k->writebytecount += bytes_written;
if(k->writebytecount == data->state.infilesize) {
/* we have sent all data we were supposed to */
k->upload_done = TRUE;
infof(data, "We are completely uploaded and fine\n");
}

Yet in the success case (I used 475021) that's never hit (shouldn't be anyway in my opinion), it just keeps uploading:

bytes: 16384
fillcount: 16380
k->writebytecount = k->writebytecount + bytes_written (442260 + 16380)
k->writebytecount: 458640
bytes: 16384
fillcount: 16380
k->writebytecount = k->writebytecount + bytes_written (458640 + 16380)
k->writebytecount: 475020
bytes: 16384
fillcount: 239
k->writebytecount = k->writebytecount + bytes_written (475020 + 239)
k->writebytecount: 475259
bytes: 16384
fillcount: 5
k->writebytecount = k->writebytecount + bytes_written (475259 + 5)
k->writebytecount: 475264

bytes is what's going in to fillreadbuffer and fillcount is what comes out. it just happens to work because k->writebytecount is never 475021 so it never caps it

so we could check chunked there and ignore infilesize in that case, or keep track of the whole thing? but if it's chunked we may not know the filesize. fillreadbuffer has some logic to deal with chunks

curl/lib/transfer.c

Lines 130 to 134 in 3ea7679

if(data->req.upload_chunky) {
/* if chunked Transfer-Encoding */
buffersize -= (8 + 2 + 2); /* 32bit hex + CRLF + CRLF */
data->req.upload_fromhere += (8 + 2); /* 32bit hex + CRLF */
}

@jay jay added the HTTP label Oct 20, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 21, 2017

Member

transfer.c:1049 (if(k->writebytecount == data->state.infilesize)) should indeed not be checked if chunked encoding is used, that looks like a blatant bug to me!
Member

bagder commented Oct 21, 2017

transfer.c:1049 (if(k->writebytecount == data->state.infilesize)) should indeed not be checked if chunked encoding is used, that looks like a blatant bug to me!
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 22, 2017

Member

how about

diff --git a/lib/transfer.c b/lib/transfer.c
index 8e66d0d..63d673e 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -1046,7 +1046,8 @@ static CURLcode readwrite_upload(struct Curl_easy *data,
 
     k->writebytecount += bytes_written;
 
-    if(k->writebytecount == data->state.infilesize) {
+    if((!data->req.upload_chunky || data->req.forbidchunk) &&
+       (k->writebytecount == data->state.infilesize)) {
       /* we have sent all data we were supposed to */
       k->upload_done = TRUE;
       infof(data, "We are completely uploaded and fine\n");

in the case of chunked afaics readbuffer sets the upload_done even though the data is just read in to the buffer, and the upload is not necessarily done.. seems kind of misleading

curl/lib/transfer.c

Lines 241 to 243 in 3ea7679

if((nread - hexlen) == 0)
/* mark this as done once this chunk is transferred */
data->req.upload_done = TRUE;

Member

jay commented Oct 22, 2017

how about

diff --git a/lib/transfer.c b/lib/transfer.c
index 8e66d0d..63d673e 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -1046,7 +1046,8 @@ static CURLcode readwrite_upload(struct Curl_easy *data,
 
     k->writebytecount += bytes_written;
 
-    if(k->writebytecount == data->state.infilesize) {
+    if((!data->req.upload_chunky || data->req.forbidchunk) &&
+       (k->writebytecount == data->state.infilesize)) {
       /* we have sent all data we were supposed to */
       k->upload_done = TRUE;
       infof(data, "We are completely uploaded and fine\n");

in the case of chunked afaics readbuffer sets the upload_done even though the data is just read in to the buffer, and the upload is not necessarily done.. seems kind of misleading

curl/lib/transfer.c

Lines 241 to 243 in 3ea7679

if((nread - hexlen) == 0)
/* mark this as done once this chunk is transferred */
data->req.upload_done = TRUE;

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 22, 2017

Member

seems kind of misleading

In fairness I found it noted in the comments.

curl/lib/urldata.h

Lines 583 to 584 in 3ea7679

bool upload_done; /* set to TRUE when doing chunked transfer-encoding upload
and we're uploading the last chunk */

Member

jay commented Oct 22, 2017

seems kind of misleading

In fairness I found it noted in the comments.

curl/lib/urldata.h

Lines 583 to 584 in 3ea7679

bool upload_done; /* set to TRUE when doing chunked transfer-encoding upload
and we're uploading the last chunk */

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 22, 2017

Member

how about

Yes, seems appropriate. Make it a PR and let's see that nothing obvious breaks from it!
Member

bagder commented Oct 22, 2017

how about

Yes, seems appropriate. Make it a PR and let's see that nothing obvious breaks from it!

jay added a commit to jay/curl that referenced this issue Oct 24, 2017

@jay
transfer: Fix chunked-encoding upload bug 
- When uploading via chunked-encoding don't compare file size to bytes
  sent to determine whether the upload has finished.

Chunked-encoding adds its own overhead which why the bytes sent is not
equal to the file size. Prior to this change if a file was uploaded in
chunked-encoding and its size was known it was possible that the upload
could end prematurely without sending the final few chunks. That would
result in a server hang waiting for the remaining data, likely followed
by a disconnect.

The scope of this bug is limited to some arbitrary file sizes which have
not been determined. One size that triggers the bug is 475020.

Bug: curl#2001
Reported-by: moohoorama@users.noreply.github.com

Closes curl#2010
7dbf335

@jay jay referenced this issue Oct 24, 2017

Closed

transfer: Fix chunked-encoding upload bug #2010

jay added a commit that referenced this issue Oct 26, 2017

@jay
transfer: Fix chunked-encoding upload bug 
- When uploading via chunked-encoding don't compare file size to bytes
  sent to determine whether the upload has finished.

Chunked-encoding adds its own overhead which why the bytes sent is not
equal to the file size. Prior to this change if a file was uploaded in
chunked-encoding and its size was known it was possible that the upload
could end prematurely without sending the final few chunks. That would
result in a server hang waiting for the remaining data, likely followed
by a disconnect.

The scope of this bug is limited to some arbitrary file sizes which have
not been determined. One size that triggers the bug is 475020.

Bug: #2001
Reported-by: moohoorama@users.noreply.github.com

Closes #2010
Loading status checks…
979d287
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 26, 2017

Member

Fixed in 979d287, thanks
Member

jay commented Oct 26, 2017

Fixed in 979d287, thanks

@jay jay closed this Oct 26, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

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