fix: Adding validation for raw data size parsing - #85
Open
IZ-sandwich wants to merge 1 commit into
Open
Conversation
elementbound
reviewed
Sep 2, 2026
elementbound
left a comment
elementbound
left a comment
Contributor
There was a problem hiding this comment.
Huge kudos for the fix! 🎉 Added some thoughts and a few questions.
| ]); | ||
| }); | ||
|
|
||
| test("should reject raw data with a malformed terminator", () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| test("should reject raw data with a malformed terminator", () => { | |
| test("should reject raw data with incorrect size", () => { |
WDYT?
|
|
||
| // The malformed terminator is consumed in its place, so parsing resumes | ||
| // on the next line | ||
| reader.ingest("\rcommand 4\n1234Xcommand foo\n"); |
Contributor
There was a problem hiding this comment.
Suggested change
| reader.ingest("\rcommand 4\n1234Xcommand foo\n"); | |
| reader.ingest("\rinvalid-command 4\n1234Xvalid-command foo\n"); |
| reader.ingest("\rcommand 4\n1234Xcommand foo\n"); | ||
| expect(() => [...reader.commands()]).toThrow(UnexpectedCharacterError); | ||
|
|
||
| expect([...reader.commands()].map((it) => it.name)).toEqual(["command"]); |
Contributor
There was a problem hiding this comment.
Suggested change
| expect([...reader.commands()].map((it) => it.name)).toEqual(["command"]); | |
| expect([...reader.commands()].map((it) => it.name)).toEqual(["valid-command"]); |
| .createServer(options, connectionListener) | ||
| .on("connection", (socket: net.Socket) => { | ||
| socket.on("data", (data: Buffer) => this.ingest(data, socket)); | ||
| socket.on("close", () => this.detach(socket)); |
Contributor
There was a problem hiding this comment.
Do we need an "error" handler too?
| ): net.Socket { | ||
| const socket = net.createConnection(options, connectionListener); | ||
| socket.on("data", (data: Buffer) => this.ingest(data, socket)); | ||
| socket.on("close", () => this.detach(socket)); |
Contributor
There was a problem hiding this comment.
Do we need an "error" handler too?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses an issue with TrimsockReader where it can be put into a state it can never leave. If queuedRawCommand (on js side, but similar issue exists with raw_command in gd) is set with a non-integer size or size > maxSize every subsequent read() returns undefined regardless of what other input is sent.
The way the reader gets into a bad state is a bad size in raw command sets queuedRawCommand but it is cleared only in the happy path so gets stuck.
I found this issue with my noray server hanging, seemingly being up but not responding to register commands. I suspect it was a crawler making a generic HTTP request containing
\r\nand bricking it. Additionally, I found that the sockets were not being detached on close or errors, so fixed that.I focused on fixing the js side since that's the used dependency of noray, but made similar changes on the godot side, exposing the ingest error as a signal.
Will need to send out a followup pr to upgrade noray to use the newer version of trimsock.