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

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

[liucheng (G)]

[-- Attachment #1: Type: text/plain, Size: 3076 bytes --]

Dear all,



The code bellow in the dladdr function has different behaviors at different optimization levels.

2219         if (bestsym && besterr > bestsym->st_size-1) {

2220                 best = 0;

2221                 bestsym = 0;

2222         }



Case of O1(arm32 little-endian):

154:         e3580000        cmp   r8, #0

158:         0a000003        beq   16c <dladdr+0x16c>

15c:         e5983008        ldr    r3, [r8, #8]

160:         e2433001        sub   r3, r3, #1

164:         e153000a        cmp   r3, sl

168:         3a000011        bcc   1bc <dladdr+0x1b4>



Case of O2:

75e00:       e5942044        ldr     r2, [r4, #68]   ; 0x44

75e04:       e2433001        sub     r3, r3, #1

75e08:       e5941004        ldr     r1, [r4, #4]

75e0c:       e1530009        cmp     r3, r9

75e10:       2a000007        bcs     75e34 <dladdr+0xfc>

75e14:       e8870006        stm     r7, {r1, r2}



In case of O2, the first part "bestsym" has been optimized, which may cause segment fault.



[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

@@ -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;

       uint32_t nsym;

       char *strings;

       size_t best = 0;

--

1.8.5.6





[testcase]

------------

#define _GNU_SOURCE

#include <link.h>

#include <stdlib.h>

#include <stdio.h>

#include <string.h>

#include <dlfcn.h>



static int callback(struct dl_phdr_info *info, size_t size, void *data)

{

        int j,ret;

        printf ("name=%s (%d segments)\n", info->dlpi_name, info->dlpi_phnum);



        if(!strcmp(info->dlpi_name,"/lib/ld-musl-arm.so.1")) {

                printf("ld-musl-arm have no indo\n");

                return 0;

        }



        for (j = 0; j < info->dlpi_phnum; j++) {

                void* addr = (void *) (info->dlpi_addr + info->dlpi_phdr[j].p_vaddr);

                printf ("\t\t header %2d: address=%10p\n", j, addr);

                Dl_info dlinfo;

                ret = dladdr(addr, &dlinfo);



                printf("\t\t\t %s : %s.", dlinfo.dli_fname, dlinfo.dli_sname);



                if((addr == NULL && ret == 0) || (addr != NULL && ret == 1)) {

                        printf(" dladdr pass return:%d\n",ret);

                } else {

                        printf(" dladdr error return:%d\n",ret);

                }

        }



        return 0;



}



int main (int argc, char *argv[])

{

        dl_iterate_phdr(callback, NULL);

        exit(EXIT_SUCCESS);

}

------------


[-- Attachment #2: Type: text/html, Size: 16054 bytes --]

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

[Rich Felker]

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

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

[liucheng (G)]

>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