Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Fix regression in IS_HEADER_CHAR check #283

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 15, 2016

Should have been checking against unsigned char as opposed to char.
bump to v2.6.2
/cc @indutny

@@ -440,7 +440,7 @@ enum http_host_state
* character or %x80-FF
**/
#define IS_HEADER_CHAR(ch) \
(ch == CR || ch == LF || ch == 9 || (ch > 31 && ch != 127))
(ch == CR || ch == LF || ch == 9 || ((unsigned char)ch > 31 && ch != 127))

This comment was marked as off-topic.

@indutny
Copy link
Member

indutny commented Feb 15, 2016

Needs test. Otherwise LGTM

@silentroach
Copy link

strange workflow, you just fixed it directly in node with more code and test.

why not just sync that changes here?

@jasnell
Copy link
Member Author

jasnell commented Feb 16, 2016

We have a release for 4.3.1 coming out later on today and I wanted to make sure the regression fix was included in that. Otherwise it would have landed here first. Unfortunately I'm tied up at a conference all day so I won't be able to get the additional tests for this implemented until tomorrow (unless someone wants to beat me to it and open a PR against my dev branch ;-) ... hint hint nudge nudge). Then we can get this updated here and get the downstream and upstream sync'd.

@jasnell
Copy link
Member Author

jasnell commented Mar 4, 2016

@indutny @jbergstroem ... test case added. I'll squash the commits down before landing. PTAL

@jasnell
Copy link
Member Author

jasnell commented Mar 4, 2016

Closed in favor of #287

@jasnell jasnell closed this Mar 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants