Issue 2228783004: Fix memory corruption from SetMaxPacingRate by inlining PacingSender.

Issue 2228783004: Fix memory corruption from SetMaxPacingRate by inlining PacingSender. (Closed)

Created:
4 years, 4 months ago by danzh1

Modified:
4 years, 4 months ago

Reviewers:
Ryan Hamilton

CC:
chromium-reviews, cbentzel+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@129351671

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Fix memory corruption from SetMaxPacingRate by inlining PacingSender. This behavior can be disabled by setting quic_disable_pacing_for_perf_tests to true. Merge internal change: 129356268 R=rch@chromium.org BUG=

Patch Set 1 #

Patch Set 2 : git pull from upper stream #

Created: 4 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+120 lines, -234 lines)			Patch
M	net/quic/core/congestion_control/pacing_sender.h	View	1 chunk	+21 lines, -42 lines	0 comments	Download
M	net/quic/core/congestion_control/pacing_sender.cc	View	7 chunks	+28 lines, -73 lines	0 comments	Download
M	net/quic/core/congestion_control/pacing_sender_test.cc	View	4 chunks	+17 lines, -72 lines	0 comments	Download
M	net/quic/core/quic_sent_packet_manager.h	View	3 chunks	+13 lines, -2 lines	0 comments	Download
M	net/quic/core/quic_sent_packet_manager.cc	View	9 chunks	+38 lines, -42 lines	0 comments	Download
M	net/quic/core/quic_session_test.cc	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	net/quic/test_tools/quic_connection_peer.cc	View	1 chunk	+1 line, -2 lines	0 comments	Download
M	net/quic/test_tools/quic_sent_packet_manager_peer.cc	View	1 chunk	+1 line, -1 line	0 comments	Download

Depends on Patchset:

Issue 2229023002 Patch 20001

Dependent Patchsets:

Issue 2233453002 Patch 20001

Messages

Total messages: 3 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

lgtm

danzh1

4 years, 4 months ago (2016-08-09 21:05:53 UTC) #3

Description was changed from

==========
Fix memory corruption from SetMaxPacingRate by inlining PacingSender.

This behavior can be disabled by setting
quic_disable_pacing_for_perf_tests to
true.

Note to Chromium merge sheriff: do not merge any comments after this
line.

I had previously changed the struct layout of PacingSender. This caused
the max_pacing_rate_ member to land at the same struct offset as rtt_stats_
in BbrTcpSender. This exposed a latent bug where the following sequence
caused SetMaxPacingRate to become a congestion control corruption laser:
1. QuicConnection initializes, calls QuicSentPacketManager::SetFromConfig,
enables pacing, and sets using_pacing_ = true.
2. QuicConnection gets a config; the configuration asks for a non-default
congestion control algorithm and replaces send_algorithm_. EnablePacing is
called again, but does not replace send_algorithm_ because using_pacing_ is
true.
3. SetMaxPacingRate is called from Jetstream. using_pacing_ has been true, but
send_algorithm_ now points at a non-PacingSender. SetMaxPacingRate breaks
the congestion control algorithm in some way.

With my change, \#3 causes segfaults. Previously, it would cause some other
struct member to get turned into garbage.

The fix is to remove the need for layering and to use a direct PacingSender
member.

I'm leaving using_pacing_ in for eventual support of PCC.

Merge internal change: 129356268

R=rch@chromium.org
BUG=
==========

to

==========
Fix memory corruption from SetMaxPacingRate by inlining PacingSender.

This behavior can be disabled by setting
quic_disable_pacing_for_perf_tests to
true.

Merge internal change: 129356268

R=rch@chromium.org
BUG=
==========

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages