Skip to content

Conversation

flovent
Copy link

@flovent flovent commented Aug 23, 2025

Detect address-of token in getBufferSize() and get the underlying variable's corresponding buffer size.

stringNotZeroTerminated() also calls getBuffersize(), so it will also benefit.

@chrchr-github
Copy link
Collaborator

Thanks for your contribution.
How do we handle &buf[i]?
Can you fix the error message, so that it only mentions the buffer itself?

@flovent
Copy link
Author

flovent commented Aug 23, 2025

How do we handle &buf[i]?

This patch didn't consider that because getBufferSize can't deal buf[i] without address-of, i think we need to add more logic after we get the variable in this function to handle this.

Can you fix the error message, so that it only mentions the buffer itself?

Should we remove & in all the testcases or only when variable is a pointer/array buffer, i think it might be reasonable to keep that for variables like int i;.

@chrchr-github
Copy link
Collaborator

chrchr-github commented Aug 23, 2025

How do we handle &buf[i]?

This patch didn't consider that because getBufferSize can't deal buf[i] without address-of, i think we need to add more logic after we get the variable in this function to handle this.

Ok, so maybe we should add a negative test/TODO for that case?

Can you fix the error message, so that it only mentions the buffer itself?

Should we remove & in all the testcases or only when variable is a pointer/array buffer, i think it might be reasonable to keep that for variables like int i;.

Ideally we would say 'Variable' for &i and just mention c (without &) if it is a buffer/array.

@flovent
Copy link
Author

flovent commented Aug 23, 2025

getBufferSize can't deal buf[i] without address-of

I mean something like this, it's not triggered now.

void f() {
  int* i[10];
  memset(i[1], 0, 1000);
}

And also, there is a TODO before getBufferSize, it only handles variables now, not for some complicated expression i think:

// TODO: strcpy(buf+10, "hello");
const ValueFlow::Value bufferSize = getBufferSize(argtok);

if (!tok->isUnaryOp("&"))
return tok;

const auto* op = tok->astOperand1();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use Token here since it's not much longer than auto.

Copy link

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

could you run the tools/test-my-pr.py script please? to see what warnings this will produce and then we can see if there are false positives.

@@ -553,6 +561,8 @@ ValueFlow::Value CheckBufferOverrun::getBufferSize(const Token *bufTok) const
{
if (!bufTok->valueType())
return ValueFlow::Value(-1);
if (bufTok->isUnaryOp("&"))
Copy link
Owner

Choose a reason for hiding this comment

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

just skipping the address of like this feels dangerous spontanously to me. It feels like there could be false positives.

@danmar
Copy link
Owner

danmar commented Aug 28, 2025

could you run the tools/test-my-pr.py script please? to see what warnings this will produce and then we can see if there are false positives.

try to run it yourself first. if you have trouble to run it maybe we can run it on some vm.. maybe you can run on 1000 packages or something like that..

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.

3 participants