[PATCH] utf8: NO_ICONV: silence uninitialized variable warning

Eric Sunshine· Jun 5, 2015, 06:42 UTC · lore

The last argument of reencode_string_len() is an 'int *' which is assigned the length of the converted string. When NO_ICONV is defined, however, reencode_string_len() is stubbed out by the macro:

    #define reencode_string_len(a,b,c,d,e) NULL

which never assigns a value to the final argument. When called like this:

    int n;
    char *s = reencode_string_len(..., &n);
    if (s)
        do_something(s, n);

some compilers complain that 'n' is used uninitialized within the conditional.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 utf8.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to utf8.h +3 −1

diff --git a/utf8.h b/utf8.h
index e7b2aa4..5a9e94b 100644
--- a/utf8.h
+++ b/utf8.h
@@ -31,7 +31,9 @@ char *reencode_string_len(const char *in, int insz,
 			  const char *in_encoding,
 			  int *outsz);
 #else
-#define reencode_string_len(a,b,c,d,e) NULL
+static inline char *reencode_string_len(const char *a, int b,
+					const char *c, const char *d, int *e)
+{ if (e) *e = 0; return NULL; }
 #endif
 
 static inline char *reencode_string(const char *in,

-- 
2.4.2.613.g328fd50

Jeff King· Jun 5, 2015, 09:23 UTC · re: Eric Sunshine · lore

Re: [PATCH] utf8: NO_ICONV: silence uninitialized variable warning

On Fri, Jun 05, 2015 at 02:42:16AM -0400, Eric Sunshine wrote:

Show 16 quoted lines

> The last argument of reencode_string_len() is an 'int *' which is
> assigned the length of the converted string. When NO_ICONV is defined,
> however, reencode_string_len() is stubbed out by the macro:
> 
>     #define reencode_string_len(a,b,c,d,e) NULL
> 
> which never assigns a value to the final argument. When called like
> this:
> 
>     int n;
>     char *s = reencode_string_len(..., &n);
>     if (s)
>         do_something(s, n);
> 
> some compilers complain that 'n' is used uninitialized within the
> conditional.

Hmm, this sounded familiar to me. And indeed:

  http://article.gmane.org/gmane.comp.version-control.git/264095

but I guess we never pushed that topic forward.

Show 5 quoted lines

> -#define reencode_string_len(a,b,c,d,e) NULL
> +static inline char *reencode_string_len(const char *a, int b,
> +					const char *c, const char *d, int *e)
> +{ if (e) *e = 0; return NULL; }
>  #endif

In the non-NO_ICONV code path we do not set "e" when returning NULL, so this actually behaves differently than the real function. I'm not puzzled that the compiler behaves differently (after all, it must assume that the out-parameter is filled in when it is passed to an extern function, but with the noop here it can easily see that it is not).

But I am puzzled that the compiler, when given the full code, does not realize that "s" in your example is always NULL, and the conditional is dead code.

In the patch linked above, I just initialized the variables in the callers. Your patch here just unconditionally sets the outsz parameter. That is certainly fine when NO_ICONV is set (and nicer, because it contains the fix to one spot), but I wonder if it means we are papering over a problem in the normal code path. I.e., now that we give the compiler the extra information about the implementation of reencode_string_len, it knows to complain.

But I just cannot see any way for it to be a problem. The simple code example you gave above is quite accurate.

So I think your patch is the best option, but it might be good to give one more look at the callers to be sure we are not missing something.

-Peff

Junio C Hamano· Jun 5, 2015, 22:19 UTC · re: Jeff King · lore

Re: [PATCH] utf8: NO_ICONV: silence uninitialized variable warning

Jeff King <peff@peff.net> writes:

> So I think your patch is the best option, but it might be good to give
> one more look at the callers to be sure we are not missing something.

The two callers both leave the outsz uninitialized and to a human it is obvious that uninitialized outsz is never used when the function or macro returns NULL. It is somewhat sad that we need a useless assignment to *e to help less than intelligent compilers, but it is possible that a clever compiler may be able to optimize the useless assignment away so it might even out in the end ;-)

Will queue.

Thanks.

patchutf8: NO_ICONV: silence uninitialized variable warning