My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 100390: Add CHECKs to make sure IOBuffers are not re-used following cancellation
1 person starred this issue and may be notified of changes. Back to list
 
Project Member Reported by eroman@chromium.org, Oct 14, 2011
The ownership of net::IOBuffer is supposed to be transferred upon cancelling requests that were using it.

If callers DO try to re-use an IOBuffer which is still in used by a cancelled request, bad things may happen (i.e. memory corruption).

We should add CHECKs to detect these dangerous re-uses. This can be done by introducing the concept of an IOBuffer being "locked" for usage, and asserting any time it is re-used while already locked.
Oct 14, 2011
#1 rvargas@chromium.org
It sounds kind of hard to enforce unless we move to a significantly different model.

We could add a concept of "owner" but I have a hard time visualizing how would that work in practice. The way I see it is that the API that uses an IOBuffer is the one that determines what is the right behavior. It is kind of expecting an int to have embedded facilities to make sure that caller and callee of an API follow a contract.

Maybe the problem is that having a ref cunt is an implementation detail that is being leaked to the users (so they may assume something that is not true), but fixing that also seems not trivial.
Cc: rvargas@chromium.org
Oct 14, 2011
#2 wtc@chromium.org
(No comment was entered for this change.)
Cc: wtc@chromium.org
Oct 17, 2011
#3 wtc@chromium.org
eroman,rvargas: let's discuss and see if we can come up with a good
solution.
Status: Assigned
Owner: rvargas@chromium.org
Oct 18, 2011
#4 eroman@chromium.org
Well, so I was thinking we would add those assertions directly into the implementations code which we know relies on the buffer staying alive post-cancellation.

It isn't fool-proof obviously since we need to add the calls to each implementation to get the benefits, but since the number of callers outweighs the number of implementations using IOBuffers it would still add benefits.

For example, IOBuffer::data() could be modified to CHECK(!is_locked_). Now in the interesting Cancel() methods we can set |is_locked_|. This would then catch an attempted re-use of the buffer after we expect to have passed off ownership.
Oct 18, 2011
#5 eroman@chromium.org
And similar checks for DrainableIOBuffer -- whenever re-allocating we would assert first that !is_locked
Oct 18, 2011
#6 eroman@chromium.org
I can take this.
Owner: eroman@chromium.org
Oct 20, 2011
#7 wtc@chromium.org
(No comment was entered for this change.)
Status: Started
Labels: Mstone-17
Dec 19, 2011
#8 kerz@google.com
Moving bugs marked as Started but not blockers from M17 to M18.  Please move back if you think this is a blocker, and add the ReleaseBlock-Stable label.  If you're able.
Labels: -Mstone-17 Mstone-18 MovedFrom-17
Feb 7, 2012
#9 kar...@google.com
(No comment was entered for this change.)
Labels: MovedFrom18 Mstone-19
Mar 27, 2012
#10 lafo...@google.com
(No comment was entered for this change.)
Labels: -Mstone-19 Mstone-20 MovedFrom-19
May 4, 2012
#11 dhar...@google.com
M20 is about to sail in couple of days. If this should be part of M20, add it back.
Labels: -Mstone-20 bulkmove MovedFrom-20 Mstone-21
Feb 6, 2013
#12 eroman@chromium.org
I had abandoned that CL a while ago due to lack of consensus. Updating status accordingly.
Status: WontFix
Mar 10, 2013
#13 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-Internals -Internals-Network -Mstone-21 M-21 Cr-Internals Cr-Internals-Network
Sign in to add a comment

Powered by Google Project Hosting