[PATCH] utf8: NO_ICONV: silence uninitialized variable warning

[PATCH] utf8: NO_ICONV: silence uninitialized variable warning

[Eric Sunshine]

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(-)

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

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

[Jeff King]

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

> 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.

> -#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

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

[Junio C Hamano]

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.