On 10/20/2017 06:18 PM, Martin Sebor wrote:
What might be even better would be to use the immediate uses of the
memory tag. For your case there should be only one immediate use
and it
should point to the statement which NUL terminates the destination. Or
maybe that would be worse in that you only want to allow this exception
when the statements are consecutive.
You said "maybe that would be worse" so I hadn't implemented it.
I went ahead and coded it up but with more testing I don't think
it has the desired result. See below.
I probably should have said that while it may be better at rooting out
false positives, the ability to find things that are not consecutive
could perhaps be seen as a negative in that it might fail to warn on
code that while not technically incorrect is likely dodgy.
I'll have to try this to better understand how it might work.
It's actually quite simple.
Rather than looking at the next statement in the chain via
gsi_next_nondebug you follow the def->use chain for the memory tag
associated with the string copy statement.
/* Get the memory tag that is defined by this statement. */
defvar = gimple_vdef (gsi_stmt (gsi));
imm_use_iterator iter;
gimple *use_stmt;
if (num_imm_uses (defvar) == 1)
{
imm_use_terator iter;
gimple *use_stmt;
/* Iterate over the immediate uses of the memory tag. */
FOR_EACH_IMM_USE_STMT (use_stmt, ui, defvar)
{
Check if STMT is dst[i] = '\0'
}
}
The check that there is a single immediate use is designed to make sure
you get a warning for this scenario:
Thanks for the outline of the solution. I managed to get it to
work with only a few minor changes(*) but...
strxncpy
read the destination
terminate the destination
Which I think you'd want to consider non-terminated because of the read
of the destination prior to termination.
But avoids warnings for
strxncpy
stuff that doesn't read the destination
terminate the destintion
...while it works fine for the basic cases it has the downside
of missing more subtle problems like this one:
char a[8];
void f (void) { puts (a); }
void g (const char *s)
{
strncpy (a, s);
f (); // assumes a is a string
a[sizeof a - 1] = '\0';
}
Don't you have a VDEF at the call to f()? Wouldn't that be the only
immediate use? If you hit a VDEF without finding the termination, then
you warn.
Or is it the case that the call gets inlined and we know enough about
puts that we get a VUSE rather than a VDEF?
In which case we have a VUSE at the puts and a VDEF at the array
assignment in the immediate use list.
I think this highlights that I over-simplified my pseudo code. If you
find a VUSE in the list, then you have to warn. IF you find a VDEF that
does not terminate, then you have to warn.
It's probably academic at this point -- it's probably more useful to
understand the immediate uses and the vuse/vdef work for the future. I
can live with a statement walking implementation.
or this one:
struct S { char a[8]; };
void f (const struct S *p) { puts (p->a); }
void g (struct S *p, const char *s)
{
strncpy (p->a, s);
f (p); // assumes p->a is a string
a[sizeof p->a - 1] = '\0';
}
This one looks similar.
+ if (TREE_CODE (dest) == SSA_NAME)
+ {
+ gimple *stmt = SSA_NAME_DEF_STMT (dest);
+ if (!is_gimple_assign (stmt))
+ return NULL_TREE;
+
+ dest = gimple_assign_rhs1 (stmt);
+ }
This seems wrong as-written -- you have no idea what the RHS code is.
You're just blindly taking the first operand, then digging into its
type. It probably works in practice, but it would seem better to verify
that gimple_assign_rhs_code is a conversion first (CONVERT_EXPR_CODE_P).
If it's not a CONVERT_EXPR_CODE_P, return NULL_TREE.
The code doesn't match CONVERT_EXPR_P(). It's expected to match
ADDR_EXPR and that's also what it tests for on the line just below
the hunk you pasted above. Here it is:
[ ... [
if (TREE_CODE (dest) == SSA_NAME)
{
gimple *stmt = SSA_NAME_DEF_STMT (dest);
if (!is_gimple_assign (stmt))
return NULL_TREE;
dest = gimple_assign_rhs1 (stmt);
}
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
So unless I'm missing something this does what you're looking
for.But you still making assumptions that you don't know are valid.
You walk to the defining statement of the given SSA_NAME. You verify it
is an assignment. You're OK up to this point.
You then proceed to look at rhs1. Instead you should look at
gimple_assign_rhs_code and verify that is an ADDR_EXPR.
For your particular example I don't think it matters, but I suspect it'd
matter for something if the defining statement was something like a
POINTER_PLUS_EXPR where the first operand might be an ADDR_EXPR.
Jeff
