Re: [PATCH] The local variables "sym" and "bestsym" in dladdr function are assigned initial values to NULL

[Rich Felker <dalias@libc.org>]

On Wed, Jun 19, 2019 at 07:13:18AM +0000, liucheng (G) wrote:
> Dear all,
> 
> 
> 
> The code bellow in the dladdr function has different behaviors at different optimization levels.
> 
> 2219         if (bestsym && besterr > bestsym->st_size-1) {

The underlying problem is that its behavior is undefined due to access
to an uninitialized object. This was introduced in commit
c8b49b2fbc7faa8bf065220f11963d76c8a2eb93, and seems to just be a
mistake; the condition should have been best && ..., not bestsym &&
...

> [patch]
> 
> Signed-off-by: l00383200 <liucheng32@huawei.com<mailto:liucheng32@huawei.com>>
> 
> ---
> 
> ldso/dynlink.c | 3 ++-
> 
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> 
> index 7cb66db..c5f5fb7 100644
> 
> --- a/ldso/dynlink.c
> 
> +++ b/ldso/dynlink.c

Your mail system seems to have botched the inline patch. If it can't
send clean plaintext, please use attachments.

> @@ -2175,7 +2175,8 @@ int dladdr(const void *addr_arg, Dl_info *info)
> 
> {
> 
>        size_t addr = (size_t)addr_arg;
> 
>        struct dso *p;
> 
> -        Sym *sym, *bestsym;
> 
> +       Sym *sym = NULL;
> 
> +       Sym *bestsym = NULL;

For future reference, NULL isn't used in musl style, just 0. There are
a few places it's still present but being phased out. But I think here
the right fix is probably correcting the conditional, not adding extra
initializations. Leaving the initialization out makes it possible for
static analysis tools to find bugs like the one you found. I'm kinda
surprised none (or even just normal compiler warnings) had caught it
yet.

Rich