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 header with underscore character #8475

Conversation

sirenkovladd
Copy link
Contributor

What does this PR do?

Fix response header with underscore character
Fixed: #8446

When bun receive request header with underscore request is stuck

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

  • I included a test for the new code, or existing tests cover it
  • I ran my tests locally and they pass (bun-debug test test-file-name.test)

@sirenkovladd
Copy link
Contributor Author

Fixed: #3580

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

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

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.

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

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

One comment

@rumblefrog
Copy link

Any update on merging & publishing this? This is a rather critical bug that's been opened for a while.

@sirenkovladd
Copy link
Contributor Author

@Jarred-Sumner @paperdave

@paperdave
Copy link
Collaborator

paperdave commented Feb 13, 2024

This is a rather critical bug that's been opened for a while.

we are instead going with a fix done upstream into usockets itself (which is actually much more permissive than this one was i think)

@paperdave paperdave closed this Feb 13, 2024
@sirenkovladd
Copy link
Contributor Author

Fixed in #8830

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.

Bun stucks when I use the HTTP_X_SCHEME header
4 participants