Skip to content

C++: stitch paths and ignore cast arrays in constant off-by-one query #13045

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

Merged

Conversation

rdmarsh2
Copy link
Contributor

@rdmarsh2 rdmarsh2 commented May 4, 2023

This PR does two things - stitch the first and second data flow paths together to make reviewing results easier, and then fix a false positive where casts to a differently-sized type (e.g. int[] to char*) would result in false positives.

@rdmarsh2 rdmarsh2 requested a review from a team as a code owner May 4, 2023 20:50
@github-actions github-actions bot added the C++ label May 4, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

A couple of comments, but otherwise this LGTM once we have a successful DCA run

FieldAddressToPointerArithmeticFlow::flowPath(fieldSource, sink) and
isFieldAddressSource(f, fieldSource.getNode()) and
pai.getLeft() = sink.getNode().(DataFlow::InstructionNode).asInstruction() and
pai.getElementSize() = f.getUnspecifiedType().(ArrayType).getBaseType().getSize() and
Copy link
Contributor

@MathiasVP MathiasVP May 5, 2023

Choose a reason for hiding this comment

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

I think we should push the f.getUnspecifiedType() instanceof ArrayType conjunct into the FieldAddressToPointerArithmeticConfig::isSource predicate (or even better: into the isFieldAddressSource predicate) to get a smaller isSource predicate for the first dataflow traversal.

Thinking more about this: If we push the restriction saying that f.getUnspecifiedType() must be an ArrayType into the isSource predicate we can use a flow state to remember the size of the array. Does that not mean that we would be able to handle cases like the one you added:

char *charBuf = (char*) arr->buf;
charBuf[MAX_SIZE_BYTES] = 0;

then?

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

I like this new approach, but I fear there's a potential performance problem with the current code.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

A couple of small remaining things, but otherwise this LGTM!

MathiasVP
MathiasVP previously approved these changes May 22, 2023
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM once DCA shows that performance is fine. Although, it looks like the latest DCA run still shows some performance issues?

@MathiasVP
Copy link
Contributor

I've pushed two performance-related commits @rdmarsh2 to your branch. Feel free to modify them as you see fit. I'll start a DCA run to check the performance impact of them. Hopefully that's done by the time your day starts tomorrow 🤞.

size != 0 and
size != 1
)
predicate pointerArithOverflow0(

Check warning

Code scanning / CodeQL

Candidate predicate not marked as `nomagic`

Candidate predicate to [pointerArithOverflow](1) is not marked as nomagic.
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@rdmarsh2 rdmarsh2 merged commit b2fb2aa into github:main May 26, 2023
@rdmarsh2 rdmarsh2 deleted the rdmarsh2/cpp/improve-constant-off-by-one branch May 26, 2023 17:20
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