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

lib: throws when invalid hex string in http request #45173

Closed
wants to merge 1 commit into from

Conversation

juanarbol
Copy link
Member

Signed-off-by: Juan José Arboleda [email protected]

May fix: #45150

I don't know why a string that does not follow this condition is considered an invalid hex string, but It is better to throw an exception instead of crashing the whole thing (IMO).

The condition: CHECK(str->Length() % 2 == 0 && "invalid hex string length");

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 25, 2022

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Oct 25, 2022
@juanarbol
Copy link
Member Author

cc @Flarna

What do you thing about this?

@mscdex
Copy link
Contributor

mscdex commented Oct 25, 2022

What about just throwing a JS error from C instead of aborting?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@juanarbol
Copy link
Member Author

What about just throwing a JS error from C instead of aborting?

I think that makes sense; any other reviewer w/ the same idea? I could throw from C instead.

@anonrig
Copy link
Member

anonrig commented Oct 25, 2022

I feel like this should can be a function in validators.js like validateHexString()

@Flarna
Copy link
Member

Flarna commented Oct 25, 2022

I tried to reproduce the same using net instead http but failed.

const net = require("net");
const s = net.createConnection(8000);
s.on("connect", () => {
    s.write("1", "hex");
});

net just discards the incomplete hex char.

Should we aim to be consistent?

Update:
I'm able to reproduce also with net now:

const net = require("net");
const s = net.createConnection(8000);
s.on("connect", () => {
    s.cork()
    s.write("a")
    s.write("1", "hex");
    s.uncork()
});

Seems writev aborts wheres write ignores chars.

As this is not specific to HTTP I would vote for a more generic solution like throwing a JS exception in C++ or ignore invalid chars (likely also in C++).

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Agreed, this solution should be generic and not limited to HTTP.
I think it might be better implemented in streams themselves.

@juanarbol
Copy link
Member Author

Nice! Thanks for all the input; I will transform this into a "throw from C layer" approach instead. Thanks again.

@Flarna
Copy link
Member

Flarna commented Oct 26, 2022

Thinking once more about this. An exception at this place may be hard to catch. To my understanding the data written might be buffered and passed down to native in a later tick without any usercode to place a try/catch around.

This and the fact that a single, non buffered write results in ignoring invalid data seems like it's maybe better to ignore it also in the writev case.

@juanarbol juanarbol closed this Dec 6, 2023
@juanarbol juanarbol deleted the juan/fix-http-write-crash branch December 6, 2023 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash if wrong hex string is written to http.ClientRequest
6 participants