Re: [patch] Fix BZ #16634 -- assert in ld.so when dlopen("a.out"...) is called repeatedly.

[Siddhesh Poyarekar <siddhesh dot poyarekar at gmail dot com>]

On 22 March 2014 04:09, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> I usually just use ChangeLog as commit log.
> Should I be using more verbose patch description instead?

What Mike said - it makes things much easier for us distro folks to do
backports and definitely infinitely easier for someone looking for the
rationale of a specific change.

> An application that erroneously tries to repeatedly dlopen("a.out", ...)
> hits assertion failure:
>
>   Inconsistency detected by ld.so: dl-tls.c: 474: _dl_allocate_tls_init:
>   Assertion `listp != ((void *)0)' failed!
>
> dlopen() actually fails with  "./a.out: cannot dynamically load executable",
> but it does so after incrementing dl_tls_max_dtv_idx.
>
> Once we run out of TLS_SLOTINFO_SURPLUS (62), we crash.

This is good, thanks.

>> Use __glibc_unlikely.
>
> There are a lot of __builtin_expect()s in this file.
>
> I would prefer to convert them to __glibc_{un}likely in a separate pass.

OK, but please don't forget ;)

> 2014-03-21  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>         [BZ #16634]
>         * elf/dl-load.c (open_verify): Add mode parameter.
>         Error early when ET_EXEC and mode does not have __RTLD_OPENEXEC.
>         (open_path): Change from boolean 'secure' to complete flag 'mode'
>         (_dl_map_object): Adjust.

The patch looks OK to me, but (I'm sorry it didn't occur to me in my
initial review) shouldn't there be a test case for this?  I think you
could adapt the reproducer in the bz into a test case.

Also, please try to always post the patch inline like you did
originally - an attached patch does not come up in the reply template
and due to that it is a bit cumbersome to comment on snippets of
patches.

Thanks,
Siddhesh
-- 
http://siddhesh.in