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

websockets, check for negative payload lengths #12707

Closed
wants to merge 6 commits into from

Conversation

icing
Copy link
Contributor

@icing icing commented Jan 15, 2024

  • in en- and decoding, check the websocket frame payload lengths for negative values (from curl_off_t) and error the operation in that case

@github-actions github-actions bot added the tests label Jan 15, 2024
lib/ws.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jan 15, 2024

Except that ASAN complains on the new test:

ws.c:228:52: runtime error: left shift of 255 by 56 places cannot be represented in type 'curl_off_t' (aka 'long')

- in en- and decoding, check the websocket frame payload
  lengths for negative values (from curl_off_t) and error
  the operation in that case
@jay
Copy link
Member

jay commented Jan 15, 2024

lgtm but hyper ci is failing:

test 2307...[WebSockets, overlong PING payload]

lib2302 returned 52, when expecting 23
 2307: exit FAILED

@bagder
Copy link
Member

bagder commented Jan 15, 2024

lib2302 returned 52, when expecting 23

it is really complicated sometimes to get the same error codes for hyper but 52 (CURLE_GOT_NOTHING) seems wrong.

@icing
Copy link
Contributor Author

icing commented Jan 16, 2024

lib2302 returned 52, when expecting 23

it is really complicated sometimes to get the same error codes for hyper but 52 (CURLE_GOT_NOTHING) seems wrong.

I declare test 2307 does not work with hyper at all. Nowhere do I see the server response data being processed. Change my mind.

@bagder bagder closed this in 49ca841 Jan 16, 2024
@bagder
Copy link
Member

bagder commented Jan 16, 2024

@icing: this new test 2307 seems to be flaky and has already failed for me locally and in CI builds after this...

bagder added a commit that referenced this pull request Jan 16, 2024
Added in 49ca841 (#12707) the test fails in numerous
(unrelated) CI builds.
@bagder bagder mentioned this pull request Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants