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

[Ingo Walther]

I addressed your comments, sorry for taking so long. Updated description
that reflects the rephrased warnings:

==========
This patch introduces two new warnings about bad std::string constructor
invocation. They can be turned on with -Wstring-constructor.

The first pattern it catches is:
std::string s('a', 10);

The intention probably was to call
std::string s(10, 'a');

which creates "aaaaaaaaaa". But implicit casts actually turn it into
std::string s(97, '\10');

The new warning says:

str.cc:6:17: warning: argument implicitly cast from 'char' to 'size_type'
(aka 'unsigned long') [-Wstring-constructor]
  std::string s('a', 10);
                ^~~
str.cc:6:17: note: did you mean to initialize with a repetition of a
character?
  std::string s('a', 10);
                ^~~~~~~
                10, 'a'

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.

The new warning says:

str.cc:9:24: warning: constructor call reads 10 characters going 7
characters past the end of the 3 character C-string
      constant [-Wstring-constructor]
  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:22: warning: constructor call reads 10 characters going 9
characters past the end of the 1 character C-string
      constant [-Wstring-constructor]
  std::string u("a", 10);
                ~~~  ^~
str.cc:8:22: note: did you mean to initialize with a repetition of a
character?
  std::string u("a", 10);
                ~~~~~^~
                10, 'a'

==========


On Wed, Dec 7, 2011 at 15:30, Manuel Klimek <klimek at google.com> wrote:

> A few nits.
>
> +  const StringLiteral *ExprAsStringLiteral =+
> dyn_cast<StringLiteral>(Expression);
> Here and elsewhere: 2-space indent
>
> +  // Expr might be referencing a string literal.
> +  const DeclRefExpr *DRE  = dyn_cast<DeclRefExpr>(Expression);
>
> double-space before '='
>
> +  if (const ArrayType *AT = s.Context.getAsArrayType(T)) {
> +    IsConstant = AT->getElementType().isConstant(s.Context);
> +  } else if (const PointerType *PT = T->getAs<PointerType>()) {
> +    IsConstant = T.isConstant(s.Context) &&
> +        PT->getPointeeType().isConstant(s.Context);
>
> Why is T.isConstant(...) needed here? Could you answer that in a
> comment? (I think it's non-obvious)
>
> +  S.Diag(Arg0->getExprLoc(),
> +         diag::warn_std_string_ctor_out_of_bounds)
> +     << LiteralLength << SecondArgValue.toString(10)
> +     << Arg0->getSourceRange() << Arg1->getSourceRange();
>
> Strange indent...
>
> +static void checkBasicStringConstructorCharToIntCast(
> +    Sema &S, SourceLocation ConstructLoc, CXXConstructorDecl *Constructor,
> +    MultiExprArg ExprArgs) {
> +  const QualType ParamType0 = Constructor->getParamDecl(0)->getType();
> +  const QualType ParamType1 = Constructor->getParamDecl(1)->getType();
> +  if (!ParamType0->isIntegerType()) return;
> +  if (!ParamType1->isAnyCharacterType()) return;
> +  // This is a call to the constructor  [cf.  C++11 [string.cons]
> 21.4.2/9 ]:
> +  // basic_string(size_type n, char c, const _Alloc& a = _Alloc());
> +  // If the constructor is called with a char literal as n and an integer
> as
> +  // c, this is almost certainly unintended.
>
> I don't see where the second condition "integer as c" is checked... If
> that's not a condition, I wouldn't mention it.
>
> Cheers,
> /Manuel
>
> On Mon, Dec 5, 2011 at 6:08 PM, Ingo Walther <ingow at google.com> wrote:
> > This patch introduces two new warnings about bad std::string constructor
> > invocation. They can be turned on with -Wstring-constructor.
> >
> > The first pattern it catches is:
> > std::string s('a', 10);
> >
> > The intention probably was to call
> > std::string s(10, 'a');
> >
> > which creates "aaaaaaaaaa". But implicit casts actually turn it into
> > std::string s(97, '\10');
> >
> > The new warning says:
> >
> > str.cc:6:17: warning: argument implicitly cast from 'char' to 'size_type'
> > (aka 'unsigned long') [-Wstring-constructor]
> >   std::string s('a', 10);
> >                 ^~~
> > str.cc:6:17: note: did you mean to swap the two arguments?
> >   std::string s('a', 10);
> >                 ^~~~~~~
> >                 10, 'a'
> >
> >
> > 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.
> >
> > 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]
> >   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'
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111231/42bcb52c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wstring-constructor.patch
Type: text/x-patch
Size: 16630 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111231/42bcb52c/attachment.bin>