[cfe-commits] r149268 - in /cfe/trunk: lib/Sema/SemaChecking.cpp test/SemaObjC/format-strings-objc.m

[Eli Friedman]

On Mon, Jan 30, 2012 at 4:02 PM, Jean-Daniel Dupas
<devlists at shadowlab.org> wrote:
>
> Le 30 janv. 2012 à 22:10, Eli Friedman a écrit :
>
>
> On Mon, Jan 30, 2012 at 11:46 AM, Jean-Daniel Dupas
> <devlists at shadowlab.org> wrote:
>
> Author: jddupas
>
> Date: Mon Jan 30 13:46:17 2012
>
> New Revision: 149268
>
>
> URL: http://llvm.org/viewvc/llvm-project?rev=149268&view=rev
>
> Log:
>
> Disable "non literal format string" for NSString that result from a macro
> expansion.
>
> This is to prevent diagnostic when using NSLocalizedString or
> CFCopyLocalizedString
>
> macros which are usually used in place of NS and CF strings literals.
>
>
>
> Modified:
>
>    cfe/trunk/lib/Sema/SemaChecking.cpp
>
>    cfe/trunk/test/SemaObjC/format-strings-objc.m
>
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=149268&r1=149267&r2=149268&view=diff
>
> ==============================================================================
>
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Jan 30 13:46:17 2012
>
> @@ -1586,6 +1586,13 @@
>
>                              format_idx, firstDataArg, Type))
>
>     return;  // Literal format string found, check done!
>
>
> +  // Do not emit diag when the string param is a macro expansion and the
>
> +  // format is either NSString or CFString. This is a hack to prevent
>
> +  // diag when using the NSLocalizedString and CFCopyLocalizedString macros
>
> +  // which are usually used in place of NS and CF string literals.
>
> +  if (Type == FST_NSString && Args[format_idx]->getLocStart().isMacroID())
>
> +    return;
>
> +
>
>   // If there are no arguments specified, warn with -Wformat-security,
> otherwise
>
>   // warn only with -Wformat-nonliteral.
>
>   if (NumArgs == format_idx+1)
>
>
> Modified: cfe/trunk/test/SemaObjC/format-strings-objc.m
>
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/format-strings-objc.m?rev=149268&r1=149267&r2=149268&view=diff
>
> ==============================================================================
>
> --- cfe/trunk/test/SemaObjC/format-strings-objc.m (original)
>
> +++ cfe/trunk/test/SemaObjC/format-strings-objc.m Mon Jan 30 13:46:17 2012
>
> @@ -1,4 +1,4 @@
>
> -// RUN: %clang_cc1 -fsyntax-only -verify %s
>
> +// RUN: %clang_cc1 -Wformat-nonliteral -fsyntax-only -verify %s
>
>
>  //===----------------------------------------------------------------------===//
>
>  // The following code is reduced using delta-debugging from
>
> @@ -107,3 +107,11 @@
>
>   NSString * ns3 = ns1;
>
>   NSLog(ns3); // expected-warning {{format string is not a string literal}}}
>
>  }
>
> +
>
> +// Do not emit warnings when using NSLocalizedString
>
> +extern NSString *GetLocalizedString(NSString *str);
>
> +#define NSLocalizedString(key) GetLocalizedString(key)
>
> +
>
> +void check_NSLocalizedString() {
>
> +  [Foo fooWithFormat:NSLocalizedString(@"format"), @"arg"]; // no-warning
>
> +}
>
>
> I'm not sure this is the right approach... see also
> http://llvm.org/bugs/show_bug.cgi?id=3814 , which is essentially the
> same issue.
>
>
> gettext convention are not the same as Cocoa ones.
>
> From gettext man page:  "By convention, [msgid] is the English version of
> the message, with non-ASCII characters replaced by ASCII approximations."
> which means that the gettext macro argument should be a valid format string
> when used as printf argument.
> If so, the gettext functions declaration should use the format_arg()
> attribute which is already handled in clang.
>
> Indeed, using a recent version of gettext (which use format_arg), I get the
> correct behavior:
>
> ---- fmt.c ---
> #include <stdio.h>
> #include <libintl.h>
> #include <gettext-po.h>
>
> int test(int arg) {
> return printf(gettext("this is a format string: %s"), arg);
> }
> ---------------
>
> fmt.c:7:51: warning: format specifies type 'char *' but the argument has
> type 'int'
>         return printf(gettext("this is a format string: %s"), arg);
>                                                         ~^    ~~~

Oh, cool; didn't realize that was there.

> The problem with Cocoa localization macro is that there is not such
> convention, and it is very common to use a key that is not a version of the
> localized string at all. So even if they were declared using format_arg
> (which is not the case), it would not help.

The issue I'm worried about is that -Wformat-security is supposed to
warn about any format argument we can't reason about.  The behavior
with your patch subverts the warning; we don't know that whatever
random macro is used in the first argument is doing something safe.

-Eli