Issue metadata
Sign in to add a comment
|
Network service: DataPipeElementReader has a re-entrancy bug |
||||||||||||||||||||||||
Issue descriptionWhen a read completes asynchronously, DataPipeElementReader invokes its read callback, and then sets buf_ to nullptr. The read callback can, synchronously, trigger another read. If that read doesn't completely synchronously, it will set buf_ itself, which the DataPipeElementReader will clear after invoking the callback. Then when the second read completes asynchronously, we try and dereference the null pointer, and crash. It looks to me like no tests were added when this class was landed, so I think we need a number of them (Normal case, pipe closed unexpectedly, large amounts of data lead to multiple async reads, init called multiple times, interruption previous init call). We need to be careful to ensure adequate unit testing as we add new code paths to the network service, since integration tests will often not cover all the corner cases.
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ddd214885da516a356b276ae2b5ee990f3f8ba8f commit ddd214885da516a356b276ae2b5ee990f3f8ba8f Author: Matt Menke <mmenke@chromium.org> Date: Fri Dec 15 20:08:21 2017 NetworkService: Fix a pair of bugs in DataPipeElementReader. When a Read() call finished asynchronously, it was clearing its buf_ pointer after calling into its consumer. If its consumer called Read() again, that could have resulted in populating buf_ again, which would then incorrectly be cleared by the higher level call. Also, if Init() interrupted a previous call to Init(), the callback to the first Init() call was not being cancelled. Bug: 794330 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I71faee5bcde302acd1962e55b4f2bb75d8c31c8d Reviewed-on: https://chromium-review.googlesource.com/825843 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#524448} [modify] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/BUILD.gn [add] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/data_pipe_element_reader.cc [add] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/data_pipe_element_reader.h [add] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/data_pipe_element_reader_unittest.cc [modify] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/url_loader.cc [modify] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/network/url_loader_unittest.cc [modify] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/content/test/BUILD.gn [modify] https://crrev.com/ddd214885da516a356b276ae2b5ee990f3f8ba8f/net/base/upload_element_reader.h
,
Dec 15 2017
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a069e7d76a1119b18adfe3a08e5a894b9edcc260 commit a069e7d76a1119b18adfe3a08e5a894b9edcc260 Author: Matt Menke <mmenke@chromium.org> Date: Wed Jan 10 20:44:08 2018 Add support to SimpleURLLoader for uploading strings. Shorter strings are just included as a DataElement inline. Longer strings are streamed to the network service instead, to reduce copies and peak memory usage, at the cost of latency. Bug: 794330 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo Change-Id: I5da99e34ac29aeb40de5667cf6fdeae14c8c7d39 Reviewed-on: https://chromium-review.googlesource.com/827731 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Randy Smith <rdsmith@chromium.org> Cr-Commit-Position: refs/heads/master@{#528416} [modify] https://crrev.com/a069e7d76a1119b18adfe3a08e5a894b9edcc260/content/public/common/simple_url_loader.cc [modify] https://crrev.com/a069e7d76a1119b18adfe3a08e5a894b9edcc260/content/public/common/simple_url_loader.h [modify] https://crrev.com/a069e7d76a1119b18adfe3a08e5a894b9edcc260/content/public/common/simple_url_loader_unittest.cc [modify] https://crrev.com/a069e7d76a1119b18adfe3a08e5a894b9edcc260/net/test/embedded_test_server/default_handlers.cc |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by mmenke@chromium.org
, Dec 13 2017