Date: Mon, 4 Feb 2013 14:31:04 -0800 From: Evan Martin <evan@chromium.org> To: "J.R. Oldroyd" <fbsd@opal.com> Cc: freebsd-chromium@freebsd.org Subject: Re: IPC memory leakage on latest chromium-24.0.1312.57 Message-ID: <CAFzwtj0Zge0-h3QSSm010c7Ep5nvSHMqnmwysOgCVzSBk_MZpg@mail.gmail.com> In-Reply-To: <20130204170802.3ed0ac23@shibato> References: <CAJuc1zNLC-ESojGq9B5-_7qr6EiwGEwxUojBE9fkU0GybEr6nw@mail.gmail.com> <1563077977.10385807.1359749865202.JavaMail.root@k-state.edu> <CAF6rxgkaqOw_CwikrWOHC26YRHjx8Ade4KORasZn8WzbmYbrVQ@mail.gmail.com> <20130203122913.6c0cf4e9@shibato> <CANcjpOBFW6xzjtzx36jn_ADSj5zh0dnedXA68=e_HMtr0_EozA@mail.gmail.com> <20130204170802.3ed0ac23@shibato>
next in thread | previous in thread | raw e-mail | index | archive | help
I am the author of some of the related code, here's a brief explanation.
One process ("per tab") runs WebKit, which has no connection to X so
that a WebKit exploit won't give an attacker direct access to your
mouse/keyboard. It does all of the drawing of web page pixels into a
buffer.
This buffer is then passed to the parent browser process, which has a
connection to X, and that process passes the buffer along to the X
server.
So there are three separate processes interacting with these pixels --
WebKit, the browser, and X. Using shared memory instead of mallocs
allows us to pass these pixels between processes without making copies
(which are large enough to be slow -- imagine a large video writing
out a screen's worth of pixels at 24fps).
It's hard to pin down exactly where to free these buffers because the
whole reason they exist is to allow multiple processes to access them
asynchronously. Also, any of these processes can die and you don't
want them leaking memory after they die. A more ideal API would allow
you to ref-count this memory, so when all the involved processes are
gone it would just free itself. The kernel toggle effectively allows
this sort of ref-counting.
Unfortunately, part of the reason this area is so complicated is that
the X APIs for shared memory require the SYSV shmat() interface,
rather than the more convenient BSD-derived mmap() interface (which is
what the rest of Chrome uses when it needs shared memory for other
purposes).
On Mon, Feb 4, 2013 at 2:08 PM, J.R. Oldroyd <fbsd@opal.com> wrote:
> On Mon, 4 Feb 2013 20:55:58 +0200 George Liaskos
> <geo.liaskos@gmail.com> wrote:
>>
>> > I think the problem may be related to
>> > files/patch-ui__surface__transport_dib_linux.cc which replaces the
>> > immediate removal of the shm after attaching to it with removal in
>> > the destructor iff we're the last who is attached to it.
>> >
>> > The comment states:
>> > // On BSD we can't access the shared memory after is marked for
>> > deletion.
>> > but this is not true if kern.ipc.shm_allow_removed=1 which we are told
>> > to set in the pkg-message.
>>
>> The problem is definitely in this patch, if kern.ipc.shm_allow_removed
>> is set then the patch is not needed at all.
>> The issue here is that the sysctl is not set by default and we must /
>> should support the default configuration.
>>
>> I don't really know how to solve correctly this issue, TransportDIBs
>> are created from a static function and the shm keys are cached.
>> Simply removing the key in the destructor is not correct because there
>> are other TransportDIB instances which use the same key.
>>
>> ... and even if you handle the removal gracefully, what happens when
>> the process crashes? More leaks, that's why all other platforms allow
>> the removal immediately.
>>
>> Maybe we are correct according to the Posix spec but now i understand
>> why almost no one follows this specific behavior.
>>
>>
>> [1] http://src.chromium.org/viewvc/chrome/trunk/src/ui/surface/transport_dib_linux.cc?revision=167669&content-type=text%2Fplain
>
> Hey George,
>
> I saw you were the submitter of this patch, so thanks for chiming in.
>
> I can say that I've run without the patch for about 36-48 hours now
> and the problem has not recurred. So those wanting a simple fix can
> simply remove the patch, recompile chrome, set
> kern.ipc.shm_allow_removed=1
> and run.
>
> Since the patch breaks things, I think it needs to be replaced. A
> simple fix would be to remove the patch and add a wrapper sh script
> that checks if the sysctl is set and prints a message if not.
>
> As for a proper fix, I haven't really studied that code in detail.
> Some possible thoughts. I am not sure what is using this shm
> region... separate processes or separate threads? Could the shm
> perhaps be replaced by something else, such as a malloc'd region
> accessed through a global variable? If not, is the removal code
> in the right place in the destructor; should it perhaps be in the
> TransportDIB::Detach method instead, or somewhere else? Do we know
> why it is not being executed - is this due to the removal code not
> being reached or is it due to something crashing? If something is
> crashing (and we can't fix that problem), can we trap that event and
> remove the shm there?
>
> -jr
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFzwtj0Zge0-h3QSSm010c7Ep5nvSHMqnmwysOgCVzSBk_MZpg>