Linux: add option to use system tcmalloc (off by default)

Linux: add option to use system tcmalloc (off by default)

This changes things only for Linux distros, Google Chrome
will continue to use bundled tcmalloc.

BUG=174634

[Paweł Hajdan Jr., 2013-02-06 14:42:32]

Feel free to add more people to review - just please do that directly instead of
telling me to add more reviewers, to reduce review latency. Thanks!

[Dai Mikurube (NOT FULLTIME), 2013-02-07 04:13:55]

Just curious.  What is the purpose to use system tcmalloc?

[Paweł Hajdan Jr., 2013-02-07 09:01:29]

On 2013/02/07 04:13:55, Dai Mikurube wrote:
> Just curious.  What is the purpose to use system tcmalloc?

https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries is one of them.

[yurys, 2013-02-07 09:04:47]

devtools_agent.cc - lgtm

[Dai Mikurube (NOT FULLTIME), 2013-02-07 09:15:01]

On 2013/02/07 09:01:29, Paweł Hajdan Jr. wrote:
> On 2013/02/07 04:13:55, Dai Mikurube wrote:
> > Just curious.  What is the purpose to use system tcmalloc?
> 
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries is one of them.

+jar

I'm a little doubtful whether we should support it.  Chromium's tcmalloc is
extended a lot and may be customized more.

If Chromium depends on some customization on tcmalloc in the future, will we
keep many #if branches to support system tcmalloc?

[Paweł Hajdan Jr., 2013-02-07 09:30:05]

On 2013/02/07 09:15:01, Dai Mikurube wrote:
> On 2013/02/07 09:01:29, Paweł Hajdan Jr. wrote:
> > On 2013/02/07 04:13:55, Dai Mikurube wrote:
> > > Just curious.  What is the purpose to use system tcmalloc?
> > 
> > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries is one of
them.
> 
> +jar
> 
> I'm a little doubtful whether we should support it.  Chromium's tcmalloc is
> extended a lot and may be customized more.
> 
> If Chromium depends on some customization on tcmalloc in the future, will we
> keep many #if branches to support system tcmalloc?

Both are Google projects. What's the problem with keeping things in sync?
Actually keeping a fork of tcmalloc makes it harder for you to update. I was
able to easily upstream my changes to debugallocation.cc, in fact I did it
upstream first.

By the way, this is not a matter of discussion. This has been discussed a lot
before, and we should provide switches to use system libraries when these
libraries are packaged by Linux distros, see
http://www.chromium.org/developers/adding-3rd-party-libraries

[Dai Mikurube (NOT FULLTIME), 2013-02-07 09:46:03]

On 2013/02/07 09:30:05, Paweł Hajdan Jr. wrote:
> On 2013/02/07 09:15:01, Dai Mikurube wrote:
> > On 2013/02/07 09:01:29, Paweł Hajdan Jr. wrote:
> > > On 2013/02/07 04:13:55, Dai Mikurube wrote:
> > > > Just curious.  What is the purpose to use system tcmalloc?
> > > 
> > > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries is one of
> them.
> > 
> > +jar
> > 
> > I'm a little doubtful whether we should support it.  Chromium's tcmalloc is
> > extended a lot and may be customized more.
> > 
> > If Chromium depends on some customization on tcmalloc in the future, will we
> > keep many #if branches to support system tcmalloc?
> 
> Both are Google projects. What's the problem with keeping things in sync?
> Actually keeping a fork of tcmalloc makes it harder for you to update. I was
> able to easily upstream my changes to debugallocation.cc, in fact I did it
> upstream first.
> 
> By the way, this is not a matter of discussion. This has been discussed a lot
> before, and we should provide switches to use system libraries when these
> libraries are packaged by Linux distros, see
> http://www.chromium.org/developers/adding-3rd-party-libraries

Ok, I agree with the 'use_system_foo' switches as written in the page.  "They're
not officially supported."

I would like to sync tcmalloc.  I actually know the pain.  (I spent 2 months to
roll Chromium's tcmalloc r109->r144!)  I then did some work to reduce the
difference.  The problem is that the original tcmalloc assumes server-like
usage, and Chromium's tcmalloc customizes and tunes for client usage.

also fyi, tcmalloc (gperftools) is not a Google project now. 
https://code.google.com/p/gperftools/  It's harder to sync. It's hard add
experimental things without forking.

[Paweł Hajdan Jr., 2013-02-18 10:01:16]

TBR

1. No objections from people who commented here.
2. No change for Google Chrome.
3. Standard change consistent with third party libraries policy.
4. No response to code review ping.

I'm happy to make any necessary post-review changes if needed.

[jschuh, 2013-02-18 11:16:26]

On 2013/02/18 10:01:16, Paweł Hajdan Jr. wrote:
> TBR
> 
> 1. No objections from people who commented here.
> 2. No change for Google Chrome.
> 3. Standard change consistent with third party libraries policy.
> 4. No response to code review ping.
> 
> I'm happy to make any necessary post-review changes if needed.

I don't see sign off Jim or Dai on this CL, and I know for a fact that our
TCMalloc is so significantly branched from the default TCMalloc that this is a
very bad idea. So, my inclination is to revert this for making abuse of TBR.

[Alexander Potapenko, 2013-02-18 11:48:40]

+Julien, who's recently added a couple of security patches to tcmalloc (see
https://code.google.com/p/chromium/issues/detail?id=169327 and
https://code.google.com/p/chromium/issues/detail?id=170133)

[jschuh, 2013-02-18 12:08:43]

I've reverted this CL. Just to provide more context for the revert, from the
security team's work alone I know Chromium relies on a number of
security-specific changes to TCMalloc including: checked doubly-linked lists,
free list poisoning, metadata guarding, and contiguous allocation capping. I'm
also aware of large scale changes made for performance. So, rather than
encourage Linux distros to ship a grossly sub-par and less secure port, I really
think this one needs to be kept as is.

[jln (very slow on Chromium), 2013-02-19 22:04:35]

On 2013/02/18 12:08:43, Justin Schuh wrote:
> I've reverted this CL. Just to provide more context for the revert, from the
> security team's work alone I know Chromium relies on a number of
> security-specific changes to TCMalloc including: checked doubly-linked lists,
> free list poisoning, metadata guarding, and contiguous allocation capping. I'm
> also aware of large scale changes made for performance. So, rather than
> encourage Linux distros to ship a grossly sub-par and less secure port, I
really
> think this one needs to be kept as is.

Also this should not pass our security unittests (part of base_unittests). Did
it ? I can't find any log in the try bots.

Unfortunately, these tests disable themselves when NO_TCMALLOC, for instance, is
set to allow certain testing configuration to work.

[jar (doing other things), 2013-02-20 17:28:19]

I'd like to see the security team happy with this before landing it.

From a performance perspective, I think that using a system library would hurt
performance, but I'd personally rather see more Linux distros  use Chromium.  As
a result, I can live with the change if it is a requirement for them.  

...but the security team seems to feel badly that we (at Chromium) would be
endorsing a significantly weaker-ware version of Chromium if the static TCMalloc
was replaced.  They are fine with many of the other libraries, but the ones that
have too many security customizations (like TCMalloc) appear to be where the are
working to draw a line.

Distros can certainly "customize" their builds any way they want, but the
question is, how much should we change our code base to be supportive of such
customizations, if they appear inadvisable?

[darin (slow to review), 2013-02-21 04:00:46]

Is there any benefit to using system provided tcmalloc?

Downsides of this patch:

1- Chromium code gets more #ifdefs.
2- Risk that Linux distros will mistakenly use the system tcmalloc, which sounds
bad for security and performance reasons.

What am I missing?  Seems like this is a WONTFIX.

[jschuh, 2013-02-21 12:10:32]

On 2013/02/21 04:00:46, darin wrote:
> Is there any benefit to using system provided tcmalloc?
> 
> Downsides of this patch:
> 
> 1- Chromium code gets more #ifdefs.
> 2- Risk that Linux distros will mistakenly use the system tcmalloc, which
sounds
> bad for security and performance reasons.
> 
> What am I missing?  Seems like this is a WONTFIX.

That sums it up. We're not against using system libraries where they haven't
been extensively modified in very Chromium specific ways, like TCMalloc is. And
of course, nothing prevents a distro from patching this locally, but we
shouldn't be going out of our way and carrying extra code just to help them
shoot themselves in the foot.

[Paweł Hajdan Jr., 2013-02-28 01:05:12]

I'm closing this issue - at this point it is not feasible to upstream
Chromium-specific tcmalloc changes or publish the fork, and Linux distributions
building with -Dlinux_use_tcmalloc=0 because of issues with nvidia drivers
(https://code.google.com/p/chromium/issues/detail?id=177218 and
https://bugs.gentoo.org/show_bug.cgi?id=413637) are considered less of an issue
than potentially creating an impression that use_system_tcmalloc is as good as
the bundled one.