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

select: avoid a NULL deref in cwfds_add_sock #15881

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Jan 1, 2025

curl_multi_waitfds(m, NULL, ...);

=> Curl_waitfds_init(&cwfds, ufds, size);

=> Curl_waitfds_add_ps(&cwfds);

=> cwfds_add_sock(cwfds, ...);

Would then try to use the ->wfds array while set to NULL previously. This should not happen, why this is protected with an assert to trigger debug builds if it happens.

Caught by CodeSonar

Screenshot 2025-01-01 at 12-20-53 Null Pointer Dereference daily CodeSonar

curl_multi_waitfds(m, NULL, ...);

=> Curl_waitfds_init(&cwfds, ufds, size);

=> Curl_waitfds_add_ps(&cwfds);

=>   cwfds_add_sock(cwfds, ...);

Would then try to use the ->wfds array while set to NULL previously.
This should not happen, which this is now also protected with an assert
to trigger debug builds if it happens.

Caught by CodeSonar
@testclutch
Copy link

Analysis of PR #15881 at d5b7fbab:

Test 2405 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 18 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@jay
Copy link
Member

jay commented Jan 1, 2025

Would then try to use the ->wfds array while set to NULL previously. This should not happen, why this is protected with an assert to trigger debug builds if it happens.

My understanding is cwfds_add_sock already returns 1 if there is no array (count should be 0 in this case). The debugassert does not work (see CI failure) because cwfds_add_sock is expected to work with a possible NULL wfds but count is 0 so it's ok. I don't understand how codesonar thinks it could actually dereference?

/cc @chrisd-work

@bagder
Copy link
Member Author

bagder commented Jan 1, 2025

I don't understand how codesonar thinks it could actually dereference?

Presumably because it does not properly take all the additional conditions into account. I included a screenshot of its warning in the top for better explanation.

@bagder bagder closed this in af4e859 Jan 1, 2025
@bagder bagder deleted the bagder/add_sock-null branch January 1, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants