Strip trailing OWS from header values in the C parser by LuShadowX · Pull Request #13246 · aio-libs/aiohttp · GitHub
Skip to content

Strip trailing OWS from header values in the C parser - #13246

Merged
Dreamsorcerer merged 3 commits into
aio-libs:masterfrom
LuShadowX:c-parser-strip-trailing-ows
Jul 26, 2026
Merged

Strip trailing OWS from header values in the C parser#13246
Dreamsorcerer merged 3 commits into
aio-libs:masterfrom
LuShadowX:c-parser-strip-trailing-ows

Conversation

@LuShadowX

@LuShadowX LuShadowX commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What do these changes do?

llhttp strips the OWS before a header value but leaves the OWS after it, and _process_header() decodes the raw bytes as-is. So the C parser keeps trailing spaces and tabs where the Python parser strips them:

>>> feed(b"GET / HTTP/1.1\r\nHost: a\r\ntest: \t value \t \r\n\r\n").headers["test"]
'value'        # HttpRequestParserPy
'value \t '    # HttpRequestParserC

RFC 9110 section 5.5 says the OWS on both sides is excluded from the field value, and HeadersParser.parse_headers() already does bvalue.strip(b" \t") for both headers and raw_headers (there is a comment there saying raw does not mean the OWS is kept). This does the same in the C parser.

test_http_request_parser_utf8_request_line covers this for the Python parser, but it is xfailed for the C parser over an unrelated URL difference, so the gap was not visible.

Are there changes in behavior for the user?

Header values read through the C parser no longer carry trailing spaces or tabs, so both parsers now return the same thing. raw_headers changes the same way.

Is it a substantial burden for the maintainers to support this?

No, it is three lines in _process_header() and removes a difference between the two parsers.

Related issue number

None.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes — N/A, no API change
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES/ folder
Test run

Built with the Cython extensions (make generate-llhttp, make cythonize, pip install -e .).

New test, both parsers:

$ PYTHONPATH='.' pytest tests/test_http_parser.py -k whitespace_around_header_value -v
tests/test_http_parser.py::test_whitespace_around_header_value[py-parser] PASSED
tests/test_http_parser.py::test_whitespace_around_header_value[c-parser] PASSED

Same test against master, to check it catches the bug:

E       AssertionError: assert 'value \t ' == 'value'
FAILED tests/test_http_parser.py::test_whitespace_around_header_value[c-parser]
1 failed, 1 passed

Parser suite and full suite:

$ PYTHONPATH='.' pytest tests/test_http_parser.py
837 passed, 6 deselected, 3 xfailed

$ PYTHONPATH='.' pytest --numprocesses=auto
15 failed, 4932 passed, 23 skipped, 17 xfailed

The 15 failures are all tests/test_run_app.py, OSError: [Errno 48] address already in use on port 8080 on this machine. They fail the same way on master (4930 passed there, the +2 here is the new test).

Drafted with Claude Opus 5 (Claude Code); reviewed by @LuShadowX.

@pyba-chronographer pyba-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Jul 26, 2026
@codecov

codecov Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

@LuShadowX
LuShadowX marked this pull request as ready for review July 26, 2026 05:28
@codspeed

codspeed Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 83 untouched benchmarks
⏩ 83 skipped benchmarks1

Comparing LuShadowX:c-parser-strip-trailing-ows (b57f16a) with master (4ae5d49)

Open in CodSpeed

Footnotes

  1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Dreamsorcerer
Dreamsorcerer reviewed Jul 26, 2026
View reviewed changes
Comment thread aiohttp/_http_parser.pyx Outdated
@Dreamsorcerer Dreamsorcerer added backport-3.14 Trigger automatic backporting to the 3.14 release branch by Patchback robot backport-3.15 Trigger automatic backporting to the 3.15 release branch by Patchback robot labels Jul 26, 2026
@Dreamsorcerer
Dreamsorcerer merged commit 25561d9 into aio-libs:master Jul 26, 2026
51 of 52 checks passed
@patchback

patchback Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Backport to 3.14: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.14/25561d975a7a9e10eec3b381bc769737b57b3c9b/pr-13246

Backported as #13247

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback

patchback Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Backport to 3.15: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.15/25561d975a7a9e10eec3b381bc769737b57b3c9b/pr-13246

Backported as #13248

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

This was referenced Jul 26, 2026
Merged
Merged
@Dreamsorcerer

Dreamsorcerer commented Jul 26, 2026

Copy link
Copy Markdown
Member

Thanks for that.

Dreamsorcerer pushed a commit that referenced this pull request Jul 26, 2026
@patchback @LuShadowX
[PR #13246/25561d97 backport][3.15] Strip trailing OWS from header va…
90174c5 
…lues in the C parser (#13248)

**This is a backport of PR #13246 as merged into master
(25561d9).**

Co-authored-by: Shadow_Lu <xshadowlu13@gmail.com>
Dreamsorcerer pushed a commit that referenced this pull request Jul 26, 2026
@patchback @LuShadowX
[PR #13246/25561d97 backport][3.14] Strip trailing OWS from header va…
035cb73 
…lues in the C parser (#13247)

**This is a backport of PR #13246 as merged into master
(25561d9).**

Co-authored-by: Shadow_Lu <xshadowlu13@gmail.com>
@LuShadowX

LuShadowX commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Happy to help, and thanks for the quick review!

@LuShadowX

LuShadowX commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks again for the backports. If there's anything else you'd like a hand with, feel free to ping me — I still have the differential parser harness set up, so more py-vs-C mismatches are easy for me to check.

@Dreamsorcerer

Dreamsorcerer commented Jul 27, 2026

Copy link
Copy Markdown
Member

Thanks again for the backports. If there's anything else you'd like a hand with, feel free to ping me — I still have the differential parser harness set up, so more py-vs-C mismatches are easy for me to check.

Well, there are known gaps already in the tests, if you wanted to look at those.
Look for tests that use HttpRequestParserC, HttpResponseParserC, HttpRequestParserPy or HttpResponseParserPy directly, or an xfail in them.

@LuShadowX

LuShadowX commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@LuShadowX LuShadowX mentioned this pull request Jul 27, 2026
Closed
4 of 5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dreamsorcerer Dreamsorcerer Dreamsorcerer left review comments
@webknjaz webknjaz Awaiting requested review from webknjaz webknjaz is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

No one assigned

Labels

backport-3.14 Trigger automatic backporting to the 3.14 release branch by Patchback robot backport-3.15 Trigger automatic backporting to the 3.15 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LuShadowX @Dreamsorcerer