Skip to content

Conversation

MathiasVP
Copy link
Contributor

Final part of https://github.com/github/codeql-c-analysis-team/issues/209 for this iteration.

These are slightly more awkward to model since it's most natural to model some of the flow to the file-descriptor argument, but since these are passed by value (because its value isn't actually changed) they don't have an "output" value after the function returns.

@MathiasVP MathiasVP added the C++ label Feb 19, 2021
@MathiasVP MathiasVP requested a review from a team as a code owner February 19, 2021 13:07
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

A few comments / questions. Sorry if I've misunderstood anything about the sockets library, its a long time since I've used it.

readfds.fd_count = 1;
readfds.fd_array[0] = source();
select(2, &readfds, nullptr, nullptr, &timeout);
sink(&readfds); // $ ast MISSING: ir
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you talk me through what's going on with taint in the poll and select examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. In the poll case, we taint the file descriptor, and poll wait for some I/O event, and then fill the revents field with the returned event.

In the select case, we taint a file descriptor in readfds. It looks like this is normally done using the FD_SET function which we haven't modeled yet. The call to select then taints the readfds structure, which we can then use to propagate taint to other FD_ functions.

Thinking about this taint flow again, I'm actually not sure we want taint to propagate taint to the entire object returned by these functions.

Copy link
Contributor Author

@MathiasVP MathiasVP Feb 21, 2021

Choose a reason for hiding this comment

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

f908d2f removes the taint from these functions.

@MathiasVP MathiasVP force-pushed the model-bsd-sockets-part-3 branch from 50dd0c9 to f908d2f Compare February 22, 2021 07:55
@geoffw0
Copy link
Contributor

geoffw0 commented Feb 22, 2021

LGTM, though I wouldn't mind a second opinion about taint through poll and select.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Merging.

@geoffw0 geoffw0 merged commit 362c12c into github:main Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants