Various logging-related cleanups & reformatting.

Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231
Review-Url: https://codereview.chromium.org/2712823003
Cr-Commit-Position: refs/heads/master@{#480316}
Committed: https://chromium.googlesource.com/chromium/src/+/a245bd07a1e59556dc6c967a1c53c439c3dcad9e

[Wez, 2017-02-23 05:53:57]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 05:54:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-02-23 06:13:11]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These were encountered while preparing the dump-on-DCHECK patch.

BUG=596231
==========

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Some reformatting changes as per git cl format:


Some functional changes to support dump-on-DCHECK:

BUG=596231
==========

[commit-bot: I haz the power, 2017-02-23 06:14:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 06:14:57]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Wez, 2017-02-23 06:17:03]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Some reformatting changes as per git cl format:


Some functional changes to support dump-on-DCHECK:

BUG=596231
==========

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Besides reformatting and naming cleanups there is a functional change
to PartitionAllocator, to enable cookie-padding only in Debug builds,
rather than in all DCHECK-enabled builds.

BUG=596231
==========

[Wez, 2017-02-23 06:23:50]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 06:24:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-02-23 06:24:29]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 06:24:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-02-23 06:36:21]

wez@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, palmer@chromium.org

[Wez, 2017-02-23 06:36:22]

palmer: As discussed, this makes PartitionAllocator cookie-padding conditional
on Debug vs Release, rather than DCHECK-is-on.

haraken: PTAL WTF & PartitionAlloc.
dcheng: PTAL everything else.

[dcheng, 2017-02-23 08:19:26]

//base LGTM

[haraken, 2017-02-23 08:39:15]

LGTM but...

On 2017/02/23 06:36:22, Wez wrote:
> palmer: As discussed, this makes PartitionAllocator cookie-padding conditional
> on Debug vs Release, rather than DCHECK-is-on.

would you help me understand why you want to restrict the cookie-padding to
Debug builds only?

[commit-bot: I haz the power, 2017-02-23 11:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 11:13:51]

Dry run: This issue passed the CQ dry run.

[palmer, 2017-02-23 18:53:24]

> would you help me understand why you want to restrict the cookie-padding to
Debug builds only?

That's my question, too. I *think* it might be the case that some fuzzers use
Release builds with DCHECK-is-on to get some of the checks but still be
faster/smaller. I think. Let me check with the fuzzing crew.

[Wez, 2017-02-23 19:48:46]

As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
DCHECK in there is actually security-sensitive, so if someone were to
enable DCHECKs but not have them crash then we've lost that protection.

The alternatives would be to (1) fix PartitionAlloc to have explicit
sanity-checks independent of build config or (2) change the DCHECK to a
CHECK so it'll crash even in dump-on-DCHECK builds.

On 23 February 2017 at 10:53, <palmer@chromium.org> wrote:

> > would you help me understand why you want to restrict the cookie-padding
> to
> Debug builds only?
>
> That's my question, too. I *think* it might be the case that some fuzzers
> use
> Release builds with DCHECK-is-on to get some of the checks but still be
> faster/smaller. I think. Let me check with the fuzzing crew.
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Wez, 2017-02-23 19:48:47]

As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
DCHECK in there is actually security-sensitive, so if someone were to
enable DCHECKs but not have them crash then we've lost that protection.

The alternatives would be to (1) fix PartitionAlloc to have explicit
sanity-checks independent of build config or (2) change the DCHECK to a
CHECK so it'll crash even in dump-on-DCHECK builds.

On 23 February 2017 at 10:53, <palmer@chromium.org> wrote:

> > would you help me understand why you want to restrict the cookie-padding
> to
> Debug builds only?
>
> That's my question, too. I *think* it might be the case that some fuzzers
> use
> Release builds with DCHECK-is-on to get some of the checks but still be
> faster/smaller. I think. Let me check with the fuzzing crew.
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2017-02-23 21:27:03]

On 2017/02/23 19:48:47, Wez wrote:
> As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
> DCHECK in there is actually security-sensitive, so if someone were to
> enable DCHECKs but not have them crash then we've lost that protection.
> 
> The alternatives would be to (1) fix PartitionAlloc to have explicit
> sanity-checks independent of build config or (2) change the DCHECK to a
> CHECK so it'll crash even in dump-on-DCHECK builds.
> 
> On 23 February 2017 at 10:53, <mailto:palmer@chromium.org> wrote:
> 
> > > would you help me understand why you want to restrict the cookie-padding
> > to
> > Debug builds only?
> >
> > That's my question, too. I *think* it might be the case that some fuzzers
> > use
> > Release builds with DCHECK-is-on to get some of the checks but still be
> > faster/smaller. I think. Let me check with the fuzzing crew.
> >
> > https://codereview.chromium.org/2712823003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Note that the cookie-padding should not be enabled on production builds because
it affects performance and memory.

[Wez, 2017-02-24 01:04:22]

On 2017/02/23 21:27:03, haraken wrote:
> On 2017/02/23 19:48:47, Wez wrote:
> > As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
> > DCHECK in there is actually security-sensitive, so if someone were to
> > enable DCHECKs but not have them crash then we've lost that protection.
> > 
> > The alternatives would be to (1) fix PartitionAlloc to have explicit
> > sanity-checks independent of build config or (2) change the DCHECK to a
> > CHECK so it'll crash even in dump-on-DCHECK builds.
> > 
> > On 23 February 2017 at 10:53, <mailto:palmer@chromium.org> wrote:
> > 
> > > > would you help me understand why you want to restrict the cookie-padding
> > > to
> > > Debug builds only?
> > >
> > > That's my question, too. I *think* it might be the case that some fuzzers
> > > use
> > > Release builds with DCHECK-is-on to get some of the checks but still be
> > > faster/smaller. I think. Let me check with the fuzzing crew.
> > >
> > > https://codereview.chromium.org/2712823003/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Note that the cookie-padding should not be enabled on production builds
because
> it affects performance and memory.

For the purposes of dump-on-DCHECK that restriction would actually be sufficient
to "fix" things; DoD builds would have DCHECK_IS_ON() but would still not get
cookie-padding.

Rather than make every one of the PA conditionals test both, how would you feel
about something like:

#if !defined(ENABLE_PARTITION_ALLOC_DEBUG)
#if DCHECK_IS_ON() && !OFFICIAL_BUILD
#define ENABLE_PARTITION_ALLOC_DEBUG() true
#else
#define ENABLE_PARTITION_ALLOC_DEBUG() false
#endif
#endif

[palmer, 2017-02-24 02:28:42]

> As we discussed on the dump-on-DCHECK patch, Chris,

Ah yes, I had to page that back in. I had forgotten!

> the issue is that the
> DCHECK in there is actually security-sensitive, so if someone were to
> enable DCHECKs but not have them crash then we've lost that protection.
> 
> The alternatives would be to (1) fix PartitionAlloc to have explicit
> sanity-checks independent of build config or (2) change the DCHECK to a
> CHECK so it'll crash even in dump-on-DCHECK builds.

Isn't (2) basically 1 way of doing (1)? Either way, fine by me.

I ran this by the fuzzing crew and they said they do not rely on Release builds
that have DCHECK_IS_ON. So no problem there.

[palmer, 2017-02-24 02:30:32]

lgtm

[haraken, 2017-02-24 02:33:06]

Getting back to the original question, what is a reason we have to drop the
cookie-padding from DCHECK builds?

The intention of the cookie-padding is to enable the security protection on all
non-production builds.

[Wez, 2017-02-25 22:28:28]

The problem is that PartitionAlloc doesn't currently have a single place
where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
we enable cookie-padding, and rely on this DCHECK actually crashing the
calling process, rather than allowing overrun to occur.  With
dump-on-DCHECK the DCHECK won't crash, hence the problem

Also, bear in mind that many developers rely on Release builds having
more-or-less the same performance as Official builds (modulo PGO effects),
so we should be careful about introducing differences in performance based
on whether the build is "Official"/production.

On 23 February 2017 at 18:33, <haraken@chromium.org> wrote:

> Getting back to the original question, what is a reason we have to drop the
> cookie-padding from DCHECK builds?
>
> The intention of the cookie-padding is to enable the security protection
> on all
> non-production builds.
>
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Wez, 2017-02-25 22:28:29]

The problem is that PartitionAlloc doesn't currently have a single place
where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
we enable cookie-padding, and rely on this DCHECK actually crashing the
calling process, rather than allowing overrun to occur.  With
dump-on-DCHECK the DCHECK won't crash, hence the problem

Also, bear in mind that many developers rely on Release builds having
more-or-less the same performance as Official builds (modulo PGO effects),
so we should be careful about introducing differences in performance based
on whether the build is "Official"/production.

On 23 February 2017 at 18:33, <haraken@chromium.org> wrote:

> Getting back to the original question, what is a reason we have to drop the
> cookie-padding from DCHECK builds?
>
> The intention of the cookie-padding is to enable the security protection
> on all
> non-production builds.
>
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[haraken, 2017-02-26 16:14:03]

On 2017/02/25 22:28:29, Wez wrote:
> The problem is that PartitionAlloc doesn't currently have a single place
> where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
> we enable cookie-padding, and rely on this DCHECK actually crashing the
> calling process, rather than allowing overrun to occur.  With
> dump-on-DCHECK the DCHECK won't crash, hence the problem

So is the CL the only way to circumvent the problem? Then I'm okay with it.

> 
> Also, bear in mind that many developers rely on Release builds having
> more-or-less the same performance as Official builds (modulo PGO effects),
> so we should be careful about introducing differences in performance based
> on whether the build is "Official"/production.

This is a wrong assumption though. Release builds can sometimes be much slower
than official builds, so we must use official builds to measure
performance/memory.

[Wez, 2017-06-03 00:00:51]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-03 00:01:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-03 00:16:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-03 00:16:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Wez, 2017-06-17 00:59:54]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 01:00:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-17 03:27:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-17 03:27:21]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-06-18 04:22:59]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Besides reformatting and naming cleanups there is a functional change
to PartitionAllocator, to enable cookie-padding only in Debug builds,
rather than in all DCHECK-enabled builds.

BUG=596231
==========

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

BUG=596231
==========

[Wez, 2017-06-18 04:25:10]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

BUG=596231
==========

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231
==========

[Wez, 2017-06-18 04:25:17]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-06-18 04:25:18]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2712823003/#ps60001
(title: "Fix base unit-tests")

[commit-bot: I haz the power, 2017-06-18 04:25:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-18 23:26:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497759917761130,
"parent_rev": "eb339abb6c31f0f6a02d7b7038031ced69898eea", "commit_rev":
"a245bd07a1e59556dc6c967a1c53c439c3dcad9e"}

[commit-bot: I haz the power, 2017-06-18 23:26:47]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231
==========

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231

Review-Url: https://codereview.chromium.org/2712823003
Cr-Commit-Position: refs/heads/master@{#480316}
Committed:
https://chromium.googlesource.com/chromium/src/+/a245bd07a1e59556dc6c967a1c53...
==========

[commit-bot: I haz the power, 2017-06-18 23:26:49]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a245bd07a1e59556dc6c967a1c53...