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

fix: set error to Payload before dispatcher disconnect #3068

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ihciah
Copy link

@ihciah ihciah commented Jul 10, 2023

PR Type

Fix

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

Now actix-web decode body with Content-Length like that:

  1. PayloadDeocder: Use Framed in an unconventional way which maintains data length left and returns Ok(Some(Bytes)) even when data is not sufficient. Returns PayloadItem::Chunk or PayloadItem::Eof.
  2. Codec: Convert PayloadItem to Message::Chunk(Some(Bytes)) or Message::Chunk(None).
  3. poll_request: Handling Message, for Message::Chunk(Some(..)), call feed_data; for Message::Chunk(None), call feed_eof.
  4. poll: call poll_request.

However, when connection closed, we have to tell Payload. So here( https://github.com/actix/actix-web/blob/ce3af777a0/actix-http/src/h1/dispatcher.rs#L1139 ) actix-web call feed_eof. But since the content length infomation is only inside the decoder, and the outside can only sense it by waiting the Stream ends. And here the stream ends in advance when connection closed.

If the payload sender exists, it means there's still data to receive, so we can just set error here before waking the receiver.

Close #3067

@robjtede robjtede added A-http project: actix-http B-semver-norelease change that does not require a release labels Jul 19, 2023
@robjtede
Copy link
Member

I'm apprehensive to accept PRs like this without a test case that fails before the patch.

@ihciah
Copy link
Author

ihciah commented Jul 27, 2023

Here is the case I think: #3067

This bug also caused an accident in our product environment.

@ihciah ihciah force-pushed the fix-payload-eof branch 2 times, most recently from 0e70a31 to c9a6a0c Compare July 27, 2023 08:32
@ihciah
Copy link
Author

ihciah commented Jul 27, 2023

I added a test case to prove the fix. Before the patch it fails.
Also the changelog is updated.

Note: This test might be a bit verbose as I'm not familiar with some of the actix built-in utils.

@ihciah
Copy link
Author

ihciah commented Jul 27, 2023

This issue may cause server writing wrong data to db in the case of unstable network. Do you think we should publish a new version for it? Theoretically all users could be affected by this.

@ihciah
Copy link
Author

ihciah commented Aug 25, 2023

ping @robjtede Can you review it?

@robjtede robjtede modified the milestones: actix-web v4.5, actix-web v4.4 Aug 26, 2023
@robjtede robjtede removed this from the actix-web v4.4 milestone Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http project: actix-http B-semver-norelease change that does not require a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lack validation of whether request payload match content-length
2 participants