Revert of Extend mapping merge to include reserved but unused mappings.

Revert of Extend mapping merge to include reserved but unused mappings. (https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Merging empty holes in r-x mappings was originally introduced in
https://breakpad.appspot.com/7714003 to deal with the first generation
of relro packing, which could introduce holes within a .so mapping:

  [libchrome.so]
  [guard region]
  [libchrome.so]

However, the logic is broken for the case of two *different* adjacent
.so mappings with a guard region in the middle:

  [libfoo.so]
  [guard region]
  [libchrome.so]

In this case the guard region is mistakenly associated with libfoo.so,
but that is not the right thing to do. In fact, the second generation of
rerlo packing added the guard region to prevent mmaps from overlapping
and to give room for the non-zero vaddr of relro-packed libraries, which
require an anticipated load bias.

As the first generation of relro packing is not used anymore, there is
no reason to keep this buggy code, which causes failures in decoding
crashes where an arbitrary library is mapped immediately before a rerlo
packed library.

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703
BUG=chromium:499747
R=primiano@chromium.org, rmcilroy@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/17ad0c18b179c135fc5a3d2bba199c3fa4276035

[petrcermak, 2016-04-27 15:53:17]

Description was changed from

==========
Revert code for merging gaps

BUG=499747
==========

to

==========
Revert of Extend mapping merge to include reserved but unused mappings.
(https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703

BUG=chromium:499747
==========

[petrcermak, 2016-04-27 15:54:20]

petrcermak@chromium.org changed reviewers:
+ primiano@chromium.org, rmcilroy@chromium.org

[petrcermak, 2016-04-27 15:54:21]

Hi,

Please review this patch.

Thanks,
Petr

[rmcilroy, 2016-04-27 16:12:48]

LGTM, thanks.

[Primiano Tucci (use gerrit), 2016-04-27 16:14:36]

On 2016/04/27 15:54:21, petrcermak wrote:
> Hi,
> 
> Please review this patch.
> 
> Thanks,
> Petr

LGTM but please expand the commit message. It will be super useful 1 y from now.
You want to say something like:

Merging empty holes in r-x mappings was originally introduced by crrev.com/XXXX
to deal with the first generation of relro packing.
This logic, however, is broken in the case of two adjacent mappings with a guard
region in the middle ([  libfoo.so ][  guard region ][ libchrome.so]). in this
case the guard region is mistakenly associated to libfoo.so, but that is not the
right thing to do. In fact, the second generation of rerlo packing, added that
guard region just to prevent mmaps to overlap, and to give room for the non zero
vaddr of relro-packed libraries which requires an anticipated load bias.
As the first generation of relro packing is no more, there is no reason to keep
this buggy code, which causes failure of decoding crashes where a library is
mapped immediately before a rerlo packed one.

[petrcermak, 2016-04-27 16:35:11]

Description was changed from

==========
Revert of Extend mapping merge to include reserved but unused mappings.
(https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703

BUG=chromium:499747
==========

to

==========
Revert of Extend mapping merge to include reserved but unused mappings.
(https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Merging empty holes in r-x mappings was originally introduced in
https://breakpad.appspot.com/7714003 to deal with the first generation
of relro packing, which could introduce holes within a .so mapping:

  [libchrome.so]
  [guard region]
  [libchrome.so]

However, the logic is broken for the case of two *different* adjacent
.so mappings with a guard region in the middle:

  [libfoo.so]
  [guard region]
  [libchrome.so]

In this case the guard region is mistakenly associated with libfoo.so,
but that is not the right thing to do. In fact, the second generation of
rerlo packing added the guard region to prevent mmaps from overlapping
and to give room for the non-zero vaddr of relro-packed libraries, which
require an anticipated load bias.

As the first generation of relro packing is not used anymore, there is
no reason to keep this buggy code, which causes failures in decoding
crashes where an arbitrary library is mapped immediately before a rerlo
packed library.

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703

BUG=chromium:499747
==========

[petrcermak, 2016-04-27 16:43:43]

On 2016/04/27 16:14:36, Primiano Tucci wrote:
> On 2016/04/27 15:54:21, petrcermak wrote:
> > Hi,
> > 
> > Please review this patch.
> > 
> > Thanks,
> > Petr
> 
> LGTM but please expand the commit message. It will be super useful 1 y from
now.
> You want to say something like:
> 
> Merging empty holes in r-x mappings was originally introduced by
crrev.com/XXXX
> to deal with the first generation of relro packing.
> This logic, however, is broken in the case of two adjacent mappings with a
guard
> region in the middle ([  libfoo.so ][  guard region ][ libchrome.so]). in this
> case the guard region is mistakenly associated to libfoo.so, but that is not
the
> right thing to do. In fact, the second generation of rerlo packing, added that
> guard region just to prevent mmaps to overlap, and to give room for the non
zero
> vaddr of relro-packed libraries which requires an anticipated load bias.
> As the first generation of relro packing is no more, there is no reason to
keep
> this buggy code, which causes failure of decoding crashes where a library is
> mapped immediately before a rerlo packed one.

Done.

Looks like I can't land the patch because I'm not a breakpad committer. Could
either of you land it on my behalf (or let me know how I can become a committer
quickly)?

Thanks,
Petr

[Primiano Tucci (use gerrit), 2016-04-27 16:45:55]

On 2016/04/27 16:43:43, petrcermak wrote:
> On 2016/04/27 16:14:36, Primiano Tucci wrote:
> > On 2016/04/27 15:54:21, petrcermak wrote:
> > > Hi,
> > > 
> > > Please review this patch.
> > > 
> > > Thanks,
> > > Petr
> > 
> > LGTM but please expand the commit message. It will be super useful 1 y from
> now.
> > You want to say something like:
> > 
> > Merging empty holes in r-x mappings was originally introduced by
> crrev.com/XXXX
> > to deal with the first generation of relro packing.
> > This logic, however, is broken in the case of two adjacent mappings with a
> guard
> > region in the middle ([  libfoo.so ][  guard region ][ libchrome.so]). in
this
> > case the guard region is mistakenly associated to libfoo.so, but that is not
> the
> > right thing to do. In fact, the second generation of rerlo packing, added
that
> > guard region just to prevent mmaps to overlap, and to give room for the non
> zero
> > vaddr of relro-packed libraries which requires an anticipated load bias.
> > As the first generation of relro packing is no more, there is no reason to
> keep
> > this buggy code, which causes failure of decoding crashes where a library is
> > mapped immediately before a rerlo packed one.
> 
> Done.
> 
> Looks like I can't land the patch because I'm not a breakpad committer. Could
> either of you land it on my behalf (or let me know how I can become a
committer
> quickly)?
> 
> Thanks,
> Petr

I can do, remind me tomorrow morning.
Also we should run breakpad tests (make check) to see if there is any test
relying on this.

[petrcermak, 2016-04-28 13:34:19]

On 2016/04/27 16:45:55, Primiano Tucci wrote:
> On 2016/04/27 16:43:43, petrcermak wrote:
> > On 2016/04/27 16:14:36, Primiano Tucci wrote:
> > > On 2016/04/27 15:54:21, petrcermak wrote:
> > > > Hi,
> > > > 
> > > > Please review this patch.
> > > > 
> > > > Thanks,
> > > > Petr
> > > 
> > > LGTM but please expand the commit message. It will be super useful 1 y
from
> > now.
> > > You want to say something like:
> > > 
> > > Merging empty holes in r-x mappings was originally introduced by
> > crrev.com/XXXX
> > > to deal with the first generation of relro packing.
> > > This logic, however, is broken in the case of two adjacent mappings with a
> > guard
> > > region in the middle ([  libfoo.so ][  guard region ][ libchrome.so]). in
> this
> > > case the guard region is mistakenly associated to libfoo.so, but that is
not
> > the
> > > right thing to do. In fact, the second generation of rerlo packing, added
> that
> > > guard region just to prevent mmaps to overlap, and to give room for the
non
> > zero
> > > vaddr of relro-packed libraries which requires an anticipated load bias.
> > > As the first generation of relro packing is no more, there is no reason to
> > keep
> > > this buggy code, which causes failure of decoding crashes where a library
is
> > > mapped immediately before a rerlo packed one.
> > 
> > Done.
> > 
> > Looks like I can't land the patch because I'm not a breakpad committer.
Could
> > either of you land it on my behalf (or let me know how I can become a
> committer
> > quickly)?
> > 
> > Thanks,
> > Petr
> 
> I can do, remind me tomorrow morning.
> Also we should run breakpad tests (make check) to see if there is any test
> relying on this.

I built breakpad_unittests and breakpad_unittests_deps and ran the tests
(build/android/test_runner.py gtest --suite breakpad_unittests), all of which
were passing.

Primiano: Assuming no more testing is necessary, could you please land this
patch?

Thanks,
Petr

[Primiano Tucci (use gerrit), 2016-04-28 15:49:47]

Description was changed from

==========
Revert of Extend mapping merge to include reserved but unused mappings.
(https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Merging empty holes in r-x mappings was originally introduced in
https://breakpad.appspot.com/7714003 to deal with the first generation
of relro packing, which could introduce holes within a .so mapping:

  [libchrome.so]
  [guard region]
  [libchrome.so]

However, the logic is broken for the case of two *different* adjacent
.so mappings with a guard region in the middle:

  [libfoo.so]
  [guard region]
  [libchrome.so]

In this case the guard region is mistakenly associated with libfoo.so,
but that is not the right thing to do. In fact, the second generation of
rerlo packing added the guard region to prevent mmaps from overlapping
and to give room for the non-zero vaddr of relro-packed libraries, which
require an anticipated load bias.

As the first generation of relro packing is not used anymore, there is
no reason to keep this buggy code, which causes failures in decoding
crashes where an arbitrary library is mapped immediately before a rerlo
packed library.

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703

BUG=chromium:499747
==========

to

==========
Revert of Extend mapping merge to include reserved but unused mappings.
(https://breakpad.appspot.com/7714003)

Reason for revert:
It is causing breakpad crash reports to be invalid (see the associated
bug).

Merging empty holes in r-x mappings was originally introduced in
https://breakpad.appspot.com/7714003 to deal with the first generation
of relro packing, which could introduce holes within a .so mapping:

  [libchrome.so]
  [guard region]
  [libchrome.so]

However, the logic is broken for the case of two *different* adjacent
.so mappings with a guard region in the middle:

  [libfoo.so]
  [guard region]
  [libchrome.so]

In this case the guard region is mistakenly associated with libfoo.so,
but that is not the right thing to do. In fact, the second generation of
rerlo packing added the guard region to prevent mmaps from overlapping
and to give room for the non-zero vaddr of relro-packed libraries, which
require an anticipated load bias.

As the first generation of relro packing is not used anymore, there is
no reason to keep this buggy code, which causes failures in decoding
crashes where an arbitrary library is mapped immediately before a rerlo
packed library.

Original issue's description:
> Extend mapping merge to include reserved but unused mappings.
>
> When parsing /proc/pid/maps, current code merges adjacent entries that
> refer to the same library and where the start of the second is equal to
> the end of the first, for example:
>
>   40022000-40025000 r-xp 00000000 b3:11 827        /system/lib/liblog.so
>   40025000-40026000 r--p 00002000 b3:11 827        /system/lib/liblog.so
>   40026000-40027000 rw-p 00003000 b3:11 827        /system/lib/liblog.so
>
> When the system linker loads a library it first reserves all the address
> space required, from the smallest start to the largest end address, using
> an anonymous mapping, and then maps loaded segments inside that reservation.
> If the loaded segments do not fully occupy the reservation this leaves
> gaps, and these gaps prevent merges that should occur from occurring:
>
>   40417000-4044a000 r-xp 00000000 b3:11 820        /system/lib/libjpeg.so
> > 4044a000-4044b000 ---p 00000000 00:00 0
>   4044b000-4044c000 r--p 00033000 b3:11 820        /system/lib/libjpeg.so
>   4044c000-4044d000 rw-p 00034000 b3:11 820        /system/lib/libjpeg.so
>
> Where the segments that follow this gap do not contain executable code
> the failure to merge does not affect breakpad operation.  However, where
> they do then the merge needs to occur.  Packing relocations in a large
> library splits the executable segment into two, resulting in:
>
>   73b0c000-73b21000 r-xp 00000000 b3:19 786460
> /data/.../libchrome.2160.0.so
> > 73b21000-73d12000 ---p 00000000 00:00 0
>   73d12000-75a90000 r-xp 00014000 b3:19 786460
> /data/.../libchrome.2160.0.so
>   75a90000-75c0d000 rw-p 01d91000 b3:19 786460
> /data/.../libchrome.2160.0.so
>
> Here the mapping at 73d12000-75a90000 must be merged into 73b0c000-73b21000
> so that breakpad correctly calculates the base address for text.
>
> This change enables the full merge by also merging anonymous maps which
> result from unused reservation, identified as '---p' with offset 0, and
> which follow on from an executable mapping, into that executable mapping.
>
> BUG=chromium:394703

BUG=chromium:499747
R=primiano@chromium.org, rmcilroy@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/17ad0c18b179c135fc5a3d2...
==========

[Primiano Tucci (use gerrit), 2016-04-28 15:49:49]

Committed patchset #1 (id:1) manually as
17ad0c18b179c135fc5a3d2bba199c3fa4276035 (presubmit successful).