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

["liucheng (G)" <liucheng32@huawei.com>]

>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.
Indeed, the conditional statement is the key.

The first patch was created with "git format-patch -1", and I sent it to myself and then resent it to the musl mailing list.

This new patch bellow was recreated based on your advice.

Subject: [PATCH] fix segment fault due to the NULL pointer of bestsym

Signed-off-by: Cheng Liu <liucheng32@huawei.com>
---
 ldso/dynlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 7cb66db..2bd603f 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -2216,7 +2216,7 @@ int dladdr(const void *addr_arg, Dl_info *info)
                }
        }

-       if (bestsym && besterr > bestsym->st_size-1) {
+       if (best && besterr > bestsym->st_size-1) {
                best = 0;
                bestsym = 0;
        }
--
1.8.5.6


-----邮件原件-----
发件人: Rich Felker [mailto:dalias@aerifal.cx] 代表 Rich Felker
发送时间: 2019年6月20日 0:30
收件人: musl@lists.openwall.com
主题: Re: [musl] [PATCH] The local variables "sym" and "bestsym" in dladdr function are assigned initial values to NULL

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