Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/6294 zero bytes files are chunked #6568

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

eivindt
Copy link

@eivindt eivindt commented Nov 6, 2023

See bug #6294 for original bug report.

This PR provides a possible fix for this, reverting the change in #2896.

The change in #2896 definitely causes some bad side effects, since not all web servers handle "chunked" transfer encoding (i.e. some don't handle it, some don't handle it well).

Problem demonstration

Given a simple bottle application:

from bottle import run, put, request, default_app

app = default_app()

@app.put("/")
def receive_file():
    upload = request.body.read()
    for head, val in request.headers.items():
        print("%-30s:\t%s" % (head, val))
    clength = request.headers.get("Content-Length", "not-set")
    cencoding = request.headers.get("Transfer-Encoding", "not-set")
    return { 'size': len(upload), 'content-length': clength, 'transfer-encoding': cencoding }

if __name__ == '__main__':
    app.run(host='localhost', port=8880)

And a simple requests script:

import requests
import io
empty_obj = io.BytesIO(b'')
resp = requests.put('http://localhost:8880/', data=empty_obj)
print(resp.status_code)
if resp.status_code == 200:
    print(resp.json())

Working Scenario

$ python server.py
Bottle v0.12.25 server starting up (using WSGIRefServer())...
Listening on http://localhost:8880/
Hit Ctrl-C to quit.

Content-Length                :	
Content-Type                  :	text/plain
Host                          :	localhost:8880
User-Agent                    :	python-requests/2.31.0
Accept-Encoding               :	gzip, deflate, br
Accept                        :	*/*
Connection                    :	keep-alive
Transfer-Encoding             :	chunked
127.0.0.1 - - [03/Nov/2023 15:51:51] "PUT / HTTP/1.1" 200 65

Test script output:

200
{'size': 0, 'content-length': '', 'transfer-encoding': 'chunked'}

Failing Scenario

$ gunicorn -b localhost:8880 server:app
[2023-11-03 15:52:05 +0100] [1326049] [INFO] Starting gunicorn 21.2.0
[2023-11-03 15:52:05 +0100] [1326049] [INFO] Listening at: http://127.0.0.1:8880 (1326049)
[2023-11-03 15:52:05 +0100] [1326049] [INFO] Using worker: sync
[2023-11-03 15:52:05 +0100] [1326050] [INFO] Booting worker with pid: 1326050

Test script output:

400

The Original Issue

The change causing this problem in the first place was due to failing to upload data read from a subprocess pipe.

This patch adds a check to super_len that avoids returning length 0 for file handles that are not regular files.

@sigmavirus24
Copy link
Contributor

closing & reopening to trigger a new build

@sigmavirus24
Copy link
Contributor

@eivindt would you be willing to fix the lint errors?

@eivindt
Copy link
Author

eivindt commented Feb 24, 2024

Give me a couple of days and I'll fix them.

@eivindt eivindt force-pushed the bug/6294_zero_bytes_files_are_chunked branch from e1b5859 to e2bb032 Compare March 10, 2024 14:32
eivindt and others added 4 commits March 10, 2024 15:44
PUT-ing an empty file should not cause a chunked transfer.
Differentiate between data that are known to be 0 bytes
and data where size is not known.

Only data where size is not known should be transferred
as "chunked" transfers.
@eivindt eivindt force-pushed the bug/6294_zero_bytes_files_are_chunked branch 2 times, most recently from f8b0e7a to e57b722 Compare March 10, 2024 15:10
@eivindt
Copy link
Author

eivindt commented Mar 10, 2024

Sorry for the delay, there were some windows specific issues I couldn't easily fix without a windows pc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants