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 Malformed header bug #34

Merged
merged 6 commits into from
Nov 29, 2021
Merged

Fix Malformed header bug #34

merged 6 commits into from
Nov 29, 2021

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 29, 2021

This adapts the bugfix and improvements from @RolandHeinze
mscdex/dicer#22

To see that the bug exists you have to checkout the commit "fix malformed header bug, red phase", which is prepatched phase with added unit test.
Afterwards the above patch was adapted to the codebase in "fix malformed header bug, green phase". The unit test is then green.

Checklist

@Uzlopak Uzlopak changed the title Malformed payload bug Malformed header bug Nov 29, 2021
@Uzlopak Uzlopak changed the title Malformed header bug Fix Malformed header bug Nov 29, 2021
deps/dicer/lib/Dicer.js Outdated Show resolved Hide resolved
this.buffer = lines[i];
modded = true;
break;
if (h) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the intention of this rewrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see a potential regression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand neither the original code, nor the fix, which is why the change makes me pause. I guess I would need to reread carefully both the original and the change, to fully understand what is happening in both scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.rfc-editor.org/rfc/rfc2616#section-2.2

   HTTP/1.1 header field values can be folded onto multiple lines if the
   continuation line begins with a space or horizontal tab. All linear
   white space, including folding, has the same semantics as SP. A
   recipient MAY replace any linear white space with a single SP before
   interpreting the field value or forwarding the message downstream.

And for this part there is the test "Folded values" in "dicer-headerparser". If it was wrong the test there would fail.

@kibertoad
Copy link
Member

What is the perf impact of this change?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 29, 2021

No impact on performance

> @fastify/busboy@1.0.0 bench:dicer
> node deps/dicer/bench/dicer-bench-multipart-parser.js

2439.02 mb/sec
2127.66 mb/sec
4000.00 mb/sec
3846.15 mb/sec
4000.00 mb/sec
4166.67 mb/sec
4000.00 mb/sec
3846.15 mb/sec
4166.67 mb/sec
4000.00 mb/sec

@kibertoad kibertoad merged commit e376ad7 into fastify:master Nov 29, 2021
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