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.

Sign up for GitHub

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

Jump to bottom

fix header with underscore character #8475

Closed
wants to merge 5 commits into from
Closed

fix header with underscore character #8475

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
215a854
fix header with underscore character
sirenkovladd Jan 25, 2024
e83ac87
[8446] add test
sirenkovladd Jan 25, 2024
565efb3
limit for the first character
sirenkovladd Jan 25, 2024
99cb066
restore notFieldNameWord in HttpParser.h
sirenkovladd Jan 26, 2024
b76d9b1
fix first letter
sirenkovladd Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 11 additions & 22 deletions packages/bun-uws/src/HttpParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ namespace uWS
return unsignedIntegerValue;
}

static inline bool isFieldNameFirstByte(unsigned char x)
{
return ((x > '@') & (x < '[')) |
((x > 96) & (x < '{'));
}

/* RFC 9110 16.3.1 Field Name Registry (TLDR; alnum + hyphen is allowed)
* [...] It MUST conform to the field-name syntax defined in Section 5.1,
* and it SHOULD be restricted to just letters, digits,
Expand All @@ -235,42 +241,25 @@ namespace uWS
{
return (x == '-') |
((x > '/') & (x < ':')) |
((x > '@') & (x < '[')) |
((x > 96) & (x < '{'));
isFieldNameFirstByte(x) |
(x == '_') |
(x == '.');
}

static inline uint64_t hasLess(uint64_t x, uint64_t n)
{
return (((x) - ~0ULL / 255 * (n)) & ~(x) & ~0ULL / 255 * 128);
}

static inline uint64_t hasMore(uint64_t x, uint64_t n)
{
return ((((x) + ~0ULL / 255 * (127 - (n))) | (x)) & ~0ULL / 255 * 128);
}

static inline uint64_t hasBetween(uint64_t x, uint64_t m, uint64_t n)
{
return (((~0ULL / 255 * (127 + (n)) - ((x) & ~0ULL / 255 * 127)) & ~(x) & (((x) & ~0ULL / 255 * 127) + ~0ULL / 255 * (127 - (m)))) & ~0ULL / 255 * 128);
}

static inline bool notFieldNameWord(uint64_t x)
{
return hasLess(x, '-') |
hasBetween(x, '-', '0') |
hasBetween(x, '9', 'A') |
hasBetween(x, 'Z', 'a') |
hasMore(x, 'z');
}

static inline void *consumeFieldName(char *p)
{
for (; true; p += 8)
{
uint64_t word;
memcpy(&word, p, sizeof(uint64_t));
if (notFieldNameWord(word))
if (isFieldNameFirstByte(*(unsigned char *)p))
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is too many changes. The only change in the parser code should be to check for _ and .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but per request @paperdave link i also added first character check for letter only

If it's extra, I can return it

Copy link
Collaborator

Choose a reason for hiding this comment

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

i was under the assumption the parser was already this way but it wasnt, i change my mind on this. sorry

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets do just the added | to keep this as minimal as possible to reduce breaking something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will it be enough if i change to

static inline bool notFieldNameWord(uint64_t x)
        {
            return hasLess(x, '9') |
                   hasBetween(x, '9', 'A') |
                   hasBetween(x, 'Z', 'a') |
                   hasMore(x, 'z');
        }

Copy link
Contributor Author

@sirenkovladd sirenkovladd Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This will be a small enough change to not break anything, but enough to allow only letters

also a couple of cpu instructions less)
however, in my unprofessional opinion, my version (without hasLess, hasBetween, hasMore) has even fewer cpu instructions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jarred-Sumner @paperdave

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done it

{
*(p++) |= 0x20;
while (isFieldNameByte(*(unsigned char *)p))
{
*(p++) |= 0x20;
Expand Down
88 changes: 88 additions & 0 deletions test/js/node/http/node-http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1676,3 +1676,91 @@ it("#4415.4 IncomingMessage es5", () => {
IncomingMessage.call(im, { url: "/foo" });
expect(im.url).toBe("/foo");
});

it("#8446 header with underscore", done => {
const server = Server((req, res) => {
res.end('OK');
});
server.listen(0, async (_err, host, port) => {
try {
const res = await fetch(`http://localhost:${port}`, {
headers: {
"x_foo": "bar",
},
});
expect(res.status).toBe(200);
expect(await res.text()).toBe("OK");
done();
} catch (err) {
done(err);
} finally {
server.close();
}
});
})

it("#8446 header with dot", done => {
const server = Server((req, res) => {
res.end('OK');
});
server.listen(0, async (_err, host, port) => {
try {
const res = await fetch(`http://localhost:${port}`, {
headers: {
"x.foo": "bar",
},
});
expect(res.status).toBe(200);
expect(await res.text()).toBe("OK");
done();
} catch (err) {
done(err);
} finally {
server.close();
}
});
})

it("#8446 header start with minus", done => {
const server = Server((req, res) => {
res.end('OK');
});
server.listen(0, async (_err, host, port) => {
try {
const res = await fetch(`http://localhost:${port}`, {
headers: {
"-x-foo": "bar",
},
signal: AbortSignal.timeout(500),
});
expect.unreachable();
} catch (err) {
expect(err.message).toBe("The operation timed out.");
done();
} finally {
server.close();
}
});
})

it("#8446 header start with underscore", done => {
const server = Server((req, res) => {
res.end('OK');
});
server.listen(0, async (_err, host, port) => {
try {
const res = await fetch(`http://localhost:${port}`, {
headers: {
"_x-foo": "bar",
},
signal: AbortSignal.timeout(500),
});
expect.unreachable();
} catch (err) {
expect(err.message).toBe("The operation timed out.");
done();
} finally {
server.close();
}
});
})