Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(151)

Issue 2250001: Fixes an invalid read bug in ClientSocketPoolBaseHelper and cancels ConnectJobs on network changes. (Closed)

Created:
10 years, 7 months ago by willchan no longer on Chromium
Modified:
9 years, 7 months ago
Reviewers:
eroman
CC:
chromium-reviews
Base URL:
http://src.chromium.org/git/chromium.git
Visibility:
Public.

Description

Fixes an invalid read bug in ClientSocketPoolBaseHelper and cancels ConnectJobs on network changes. The bug seems to be running callbacks that delete the ClientSocketPoolBaseHelper object. We need to hold onto a self-reference in the stack frame. The new test does not crash, but will trigger invalid reads in valgrind without the fix. The fixes are overly conservative. At every entry point that can lead to releasing the last reference, we hold onto a self-reference. Not all of them can currently lead to crashes since the last operation is running the callback. But this conservative measure will help future-proof the code in case anyone adds code without thinking about it. The references are cheap anyway. BUG=44724 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48895

Patch Set 1 #

Total comments: 11

Patch Set 2 : Redo. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -0 lines) Patch
M net/socket/client_socket_pool_base.cc View 1 4 chunks +13 lines, -0 lines 0 comments Download
M net/socket/client_socket_pool_base_unittest.cc View 1 1 chunk +25 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
willchan no longer on Chromium
For a demonstration of the valgrind error before the change, see: http://build.chromium.org/buildbot/try-server/builders/linux_valgrind/builds/1365/steps/valgrind%20test:%20net/logs/stdio The only differences ...
10 years, 7 months ago (2010-05-26 01:26:13 UTC) #1
willchan no longer on Chromium
http://codereview.chromium.org/2250001/diff/1/9 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/2250001/diff/1/9#newcode613 net/socket/client_socket_pool_base.cc:613: MaybeOnAvailableSocketSlot(group_name); For the record, this is the line that ...
10 years, 7 months ago (2010-05-26 01:27:36 UTC) #2
eroman
http://codereview.chromium.org/2250001/diff/1/4 File net/http/http_network_session.cc (right): http://codereview.chromium.org/2250001/diff/1/4#newcode77 net/http/http_network_session.cc:77: // TODO(willchan): Flush |host_resolver_|. This actually doesn't seem desirable. ...
10 years, 7 months ago (2010-05-26 18:30:59 UTC) #3
willchan no longer on Chromium
I'm sort of leaning towards undoing this change and just adding the CancelAllConnectJobs() call in ...
10 years, 7 months ago (2010-05-26 21:09:02 UTC) #4
eroman
> I'm leaning towards undoing this change and just adding the > CancelAllConnectJobs() call in ...
10 years, 7 months ago (2010-05-26 23:08:00 UTC) #5
willchan no longer on Chromium
Ok, I've gotten rid of all the other changes. I've kept the bugfix, although we ...
10 years, 7 months ago (2010-05-28 00:17:20 UTC) #6
eroman
lgtm
10 years, 7 months ago (2010-05-28 01:51:48 UTC) #7
eroman
10 years, 7 months ago (2010-05-28 01:52:16 UTC) #8
nit: may want to change the title of this codereview (not exactly a reland
anymore).

Powered by Google App Engine
This is Rietveld 408576698