(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Description • 12 years ago

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.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 1 • 12 years ago

This patch fixes the issue

	Mike Hommey [:glandium]
	Comment 2 • 12 years ago

(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.

	Mike Hommey [:glandium]
	Comment 3 • 12 years ago

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.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 4 • 12 years ago

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

Okay

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 5 • 12 years ago

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

	Josh Matthews [:jdm]
	Comment 6 • 12 years ago

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

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 7 • 12 years ago

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.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 8 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 12 years ago

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