824715 - Wrong next page offset in custom linker

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

The line at http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/CustomElf.cpp#442 calculates the page boundary at or after file_end. However, if file_end is already on a page boundary, the calculated next_page is one extra page over. If mem_end is between file_end and the wrong next_page, we never allocate the extra page to accommodate mem_end and that results in seg faults.

I've only seen this happen with libmozalloc.so when using NDKr8/gold and turning on the crash reporter.

> arm-linux-androideabi-readelf -l libmozalloc.so
>
> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   LOAD           0x000000 0x00000000 0x00000000 0x00a6d 0x00a6d R E 0x8000
>   LOAD           0x000ee0 0x00008ee0 0x00008ee0 0x00120 0x00124 RW  0x8000

Note that the second segment has file_end at 0x1000 and mem_end at 0x1004.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Correctly calculate next page offset in custom linker (v1)

This patch fixes the issue

Comment 2

[Mike Hommey [:glandium]]

(In reply to Jim Chen [:jchen :nchen] from comment #0)
> The line at
> http://mxr.mozilla.org/mozilla-central/source/mozglue/linker/CustomElf.
> cpp#442 calculates the page boundary at or after file_end. However, if
> file_end is already on a page boundary, the calculated next_page is one
> extra page over. If mem_end is between file_end and the wrong next_page, we
> never allocate the extra page to accommodate mem_end and that results in seg
> faults.

mprotect() doesn't allocate.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 695757 [details] [diff] [review]
Correctly calculate next page offset in custom linker (v1)

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

::: mozglue/linker/CustomElf.cpp
@@ +438,5 @@
>     * flags. */
>    if (pt_load->p_memsz > pt_load->p_filesz) {
>      Addr file_end = pt_load->p_vaddr + pt_load->p_filesz;
>      Addr mem_end = pt_load->p_vaddr + pt_load->p_memsz;
> +    Addr next_page = ((file_end - 1) & ~(PAGE_SIZE - 1)) + PAGE_SIZE;

(file_end + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1)
would be better.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Correctly calculate next page offset in custom linker (v2)

> (file_end + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1)
> would be better.

Okay

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10b47593c4d7

Comment 6

[Josh Matthews [:jdm]]

Backed out as part of a set of patches suspected of causing permaorange Android 2.2 opt m2 and m3:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d2552e3d3ed6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e497c2e40dc6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22e38c3125e9

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Patch passes try with Android 2.2 opt m2 and m3:

https://tbpl.mozilla.org/?tree=Try&rev=dce76adea677

So I think I will push again soon.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb242f459ff

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bb242f459ff