808121 - Future Android system linker changes break Firefox linker

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

https://android-review.googlesource.com/#/c/45935/

This change essentially sets some linker internal structures read-only, which includes those we use to hook for gdb.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Ensure the pointers we change in the r_debug data are writable, which they aren't with upcoming Android system linker

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 678094 [details] [diff] [review]
Ensure the pointers we change in the r_debug data are writable, which they aren't with upcoming Android system linker

Review of attachment 678094 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/ElfLoader.cpp
@@ +591,5 @@
> +    prot = getProt((uintptr_t) &ptr);
> +    /* Pointers are aligned such that their value can't be spanning across
> +     * 2 pages. */
> +    page = (void*)((uintptr_t) &ptr & PAGE_MASK);
> +    if (!prot & PROT_WRITE)

I am pretty sure that you want this to be !(prot & PROT_WRITE).

@@ +597,5 @@
> +  }
> +
> +  ~EnsureWritable()
> +  {
> +    if (!prot & PROT_WRITE)

Ditto.

@@ +603,5 @@
> +  }
> +
> +private:
> +  int getProt(uintptr_t addr)
> +  {

This function was responsible for the grousing about lack of system calls to obtain protection bits, wasn't it? :)

@@ +609,5 @@
> +     * startAddr-endAddr rwxp */
> +    int result = 0;
> +    char buf[1024];
> +    FILE *f = fopen("/proc/self/maps", "r");
> +    while (fgets(buf, sizeof(buf), f)) {

FWIW, MapsMemoryReporter just uses fscanf here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/MapsMemoryReporter.cpp#305

Might be worth just using that instead, especially if you can figure out an easy way to just eat the rest of the line after the permission bits.

@@ +614,5 @@
> +      char *b = buf;
> +      uintptr_t startAddr, endAddr;
> +      startAddr = readAddr(b);
> +      if (*(b++) != '-')
> +        return -1;

I approve of the attempt at error checking, but ISTM that if you're really going to try to do this, just returning -1 (effectively rwx) isn't the right way to approach it.  Either propagate the error out further or just die early.

@@ +674,5 @@
>   * to adjust the l_prev value for the first element the system linker
>   * knows about. Fortunately, it doesn't use l_prev, and the first element
>   * is not ever going to be released before our elements, since it is the
>   * program executable, so the system linker should not be changing
>   * r_debug::r_map.

Looks like this reference needs to be updated?

Comment 3

[Mike Hommey [:glandium]]

Attachment: Ensure the pointers we change in the r_debug data are writable, which they aren't with upcoming Android system linker

Comment 4

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> >   * to adjust the l_prev value for the first element the system linker
> >   * knows about. Fortunately, it doesn't use l_prev, and the first element
> >   * is not ever going to be released before our elements, since it is the
> >   * program executable, so the system linker should not be changing
> >   * r_debug::r_map.
> 
> Looks like this reference needs to be updated?

Mmmm this hasn't changed.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 678649 [details] [diff] [review]
Ensure the pointers we change in the r_debug data are writable, which they aren't with upcoming Android system linker

Review of attachment 678649 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.

My comment about r_debug::r_map was mainly triggered by the methods that used to live in r_debug now living in DebuggerHelper, but leaving the comment still referring to r_debug.  It wasn't immediately obvious to me that the reference was OK; I can certainly see why you left it that way.

Comment 6

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/e587aa26326e

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 678649 [details] [diff] [review]
Ensure the pointers we change in the r_debug data are writable, which they aren't with upcoming Android system linker

Review of attachment 678649 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/ElfLoader.cpp
@@ +95,5 @@
>  
>  int
>  __wrap_dl_iterate_phdr(dl_phdr_cb callback, void *data)
>  {
> +  if (ElfLoader::Singleton.dbg)

Am I just confused or did this invert the meaning of the condition?

Comment 8

[Mike Hommey [:glandium]]

(In reply to :Ms2ger from comment #7)
> > +  if (ElfLoader::Singleton.dbg)
> 
> Am I just confused or did this invert the meaning of the condition?

Arg, you're right.

Comment 9

[Mike Hommey [:glandium]]

Thanks Ms2ger
https://hg.mozilla.org/mozilla-central/rev/8776d96f0099

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Np