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

Wip master chunkphase1 #37

Open
wants to merge 3 commits into
base: ceph-master
Choose a base branch
from

Conversation

mdw-at-linuxbox
Copy link

3 fixes here.

1 content-length vs. transfer-encoding: chunked; according to rfc 2616,
transfer-encoding chunked should supersede content-length. Later rfcs require this be treated as an error - sadly experience has shown s3 clients lag in this respect.

2 eat final crlf after last chunk header. Otherwise, the next request fails.

3 mg_set_must_close, Gives ceph/rgw a way to say this connection is hosed, do not attempt to read another request out of it.

These are all issues from,
https://tracker.ceph.com/issues/45789

@mdw-at-linuxbox
Copy link
Author

This supersedes PR #35 that I left for civetweb. I'm guessing travis will fail again. This is simplified to be just the "safe" fixes - I'll have further fixes past this that have more complicated issues.

The final chunk header of size 0 is followed by two crlf pairs.
The first ends the chunk header, and is properly digested by
civetweb.  The 2nd ends the "dummy" datablock after the last
chunk header.  It must be consumed as well, or else the
next request on this connection will fail.  This change adds
logic to properly digest the 2nd crlf pair.

Signed-off-by: Marcus Watts <[email protected]>
In some cases, ceph may detect an error that indicates
that further use of this connection is doubtful.  For instance,
mg_read() fails to return expected data, due to a chunk decoding
error.  In this case, the connection should be closed, but not
until after the error is sent.  civetweb provides low-level logic
to do this (conn->must_close); this commit provides an API
for the application to trigger that behavior.

Signed-off-by: Marcus Watts <[email protected]>
transfer-encoding = chunked vs. content-length encoding is documented
in rfc 2616 3.6.1 and rfc 7230 4.1.  In both cases, it is
documented that if transfer-encoding == 'chunked', then content-length
should not be supplied by a client, and must not be processed by a server.

The previous code in civetweb allowed content-length to override
transfer-encoding=chunked, which is clearly wrong by the standard.
This chnage corrects the order of those checks.

Signed-off-by: Marcus Watts <[email protected]>
@mdw-at-linuxbox mdw-at-linuxbox force-pushed the wip-master-chunkphase1 branch from 0f13a81 to 54426bf Compare July 28, 2020 21:32
@mdw-at-linuxbox
Copy link
Author

I had left out a commit originally and botched a refactor. I've pushed corrections for both.

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.

2 participants