Wrong condition (off-by-one error) in net/quic/core/quic_data_writer.cc
Reported by
psali...@akamai.com,
Nov 24 2017
|
|||||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.3.5 (KHTML, like Gecko) Version/11.0.1 Safari/604.3.5
Example URL:
Steps to reproduce the problem:
.
What is the expected behavior?
What went wrong?
The header for QuicDataWriter states:
// Writes |value| to the position |offset| from the start of the data.
// |offset| must be less than the current length of the writer.
bool WriteUInt8AtOffset(uint8_t value, size_t offset);
But the current implementation also allows to write after the current length of the writer.
The easiest fix is to change by:
bool QuicDataWriter::WriteUInt8AtOffset(uint8_t value, size_t offset) {
- if (offset > length_) {
+ if (offset >= length_) {
return false;
}
Did this work before? N/A
Chrome version: 63.0.3239.58 Channel: n/a
OS Version: OS X 10.12.6
Flash Version:
,
Nov 27 2017
Sorry, but I do not understand. What means "TE-end" ? The problem is that API (quic_data_writer.h = |offset| must be less) is out of sync to implementation (quic_data_writer.cc = |offset| must be less or equal). And the situation when "|offset| is equal" should not be used, therefore the implementation should be fixed. Please, could you assign it to Component Internals>Network>QUIC, similarly as in Issue 758889 ?
,
Nov 27 2017
Thank you for providing more feedback. Adding requester "krajshree@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 27 2017
,
Nov 28 2017
[+rch, +jri]: Just want to make sure this is on your radars.
,
May 25 2018
The issue seems to be out of TE-scope as it is related to .cc file. Hence, adding label TE-NeedsTriageHelp for further investigation from dev team. Thanks...!!
,
Jul 24
This method probably should be removed (instead of being fixed) as it's never used in the production code other than the QuicDataWriterTest. Renjie: can you take a look?
,
Jul 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fac0be109b7c67c04526e4a70f28f01045d90760 commit fac0be109b7c67c04526e4a70f28f01045d90760 Author: Renjie <renjietang@chromium.org> Date: Sat Jul 28 00:08:31 2018 Remove unused function and its corresponded test. No behavior change. Merge internal change: 206330851 Bug: 788385 Change-Id: I0a0823638aa9692024751a80e6470bb11ddfeb3c Reviewed-on: https://chromium-review.googlesource.com/1153387 Reviewed-by: Zhongyi Shi <zhongyi@chromium.org> Commit-Queue: Renjie Tang <renjietang@chromium.org> Cr-Commit-Position: refs/heads/master@{#578870} [modify] https://crrev.com/fac0be109b7c67c04526e4a70f28f01045d90760/net/third_party/quic/core/quic_data_writer.cc [modify] https://crrev.com/fac0be109b7c67c04526e4a70f28f01045d90760/net/third_party/quic/core/quic_data_writer.h [modify] https://crrev.com/fac0be109b7c67c04526e4a70f28f01045d90760/net/third_party/quic/core/quic_data_writer_test.cc
,
Jul 28
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by krajshree@chromium.org
, Nov 27 2017Labels: Triaged-ET Needs-Triage-M63 Needs-Feedback