[cfe-commits] Warning for bad std::string constructor invocations

[Chandler Carruth]

I haven't had a chance to look at the patch in detail, but had a few
high-level comments.

On Mon, Dec 5, 2011 at 9:08 AM, Ingo Walther <ingow at google.com> wrote:

> The second pattern the patch catches is:
> std::string v("abc", 10);
>
> The constructor will read 10 characters into the new string, but there are
> only four (including the null terminator). This reads junk if we're lucky,
> and segfaults or opens security holes if we're unlucky.
>

This is a really cool warning, but I wonder if we shouldn't distance it a
bit from the first. Technically, this isn't constrained to the string
constructor but to any function which accepts a pointer and a size. I could
envision a nice C++11 annotation for the size parameter which refers to the
pointer parameter clarifying that this is the expected size. Then anyone
could write and annotate such interfaces and get warnings.

I'm totally happy to go forward with the current version and the current
flag, but I'd like to leave some bread crumbs for the future when this can
become a generic part of -Warray-bounds enabled via attributes on
parameters to form the association.

The new warning says:
> str.cc:9:17: warning: string literal is shorter than the number of
> characters to read (3 vs 10) [-Wstring-constructor]
>

A few nit-picks on the wording here. First, it seems like your patch (at a
very distant glance) handles cases other than an explicit string literal. I
wonder if it would be better to say 'C-string constant' or something else a
touch more generic.

Second, I have trouble re-associating the '3' with the string literal's
length and the 10 with the characters read. I wonder about putting the
values directly into the message and reversing it.

"warning: constructor call reads 10 characters going 7 past the end of the
3 character string literal"

Meh, I don't really like it yet, but something in that direction I think
would help me.

Finally, I would put the location of the warning on the size rather than
the string.

  std::string v("abc", 10);
>                 ^~~~~  ~~
>
> If the literal is just one character long, we suggest to swap the
> arguments and turn the string literal into a char literal:
> str.cc:8:17: warning: string literal is shorter than the number of
> characters to read (1 vs 10) [-Wstring-constructor]
>   std::string u("a", 10);
>                 ^~~  ~~
> str.cc:8:17: note: did you mean to swap the two arguments?
>   std::string u("a", 10);
>                 ^~~~~~~
>                 10, 'a'
>

Probably want the note to also mention switching from a string literal to a
character literal.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111206/aff482e5/attachment.html>