New issue
Advanced search Search tips

Issue 657300 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Direct-leak in webrtc::NetEqImpl::InsertPacketInternal

Project Member Reported by ClusterFuzz, Oct 19 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6673791537381376

Fuzzer: libfuzzer_neteq_rtp_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  webrtc::NetEqImpl::InsertPacketInternal
  webrtc::NetEqImpl::InsertPacket
  webrtc::test::NetEqTest::Run
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=423124:423149

Minimized Testcase (0.49 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94pK8LqxvxLhhwathU4apvZnzHaktm54XY122Hg7f7UT4EA5WWlI_R1aSJige38ih3ESuglFsnYVY4taml7s8itQ8FL8Qpan2l4L8lUWsMLwflh6Dutp851sqx4PcktIwlCLFnEHtsHRkqFC3NmCaY9HAVAIQ?testcase_id=6673791537381376

Issue manually filed by: ajha

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 

Comment 1 by ajha@chromium.org, Oct 19 2016

Cc: ajha@chromium.org
Components: Blink>WebRTC
Labels: Findit-for-crash M-56 Te-Logged
Owner: hlundin@chromium.org
Status: Assigned (was: Untriaged)
Suspected CLs	Git blame below is NOT necessarily who introduced the crash nor the owner for it. Please check the code before assigning to anyone.(No CL in the regression range changed the crashing files.)

Author: kwiberg
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/b9f89a7b3e1fc6466bb6edc56302a1568f69d977
Time: Mon Jun 20 11:47:39 2016
The CL last changed line 334 of file buffer.h, which is stack frame 1.

Author: henrik.lundin@webrtc.org
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/c340881248e3908db86096a984ba25b91f0d44fd
Time: Wed Jan 30 07:37:20 2013
The CL last changed line 589 of file neteq_impl.cc, which is stack frame 1.

Author: ossu
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/30fa051b2abe500225b8bae7a35593f8ce7e0deb
Time: Thu Sep 08 11:52:55 2016
The CL last changed line 141 of file neteq_impl.cc, which is stack frame 2.

Author: henrik.lundin
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/11ab84318626e4ab9f7c22ef48ff8ab36391b0c6
Time: Wed Jun 22 13:34:03 2016
The CL last changed line 72 of file neteq_test.cc, which is stack frame 3.

Author: henrik.lundin
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/ebb2828fb18da20df1bbc78604f82d3e32669ece
Time: Wed Oct 05 09:27:42 2016
The CL last changed line 156 of file neteq_rtp_fuzzer.cc, which is stack frame 4.

Author: Peter Boström
Project: chromium-webrtc
Changelist: https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/fe3129a990da6b19e8366bf037c065e617e2c4a6
Time: Mon Nov 23 14:12:06 2015
The CL last changed line 39 of file webrtc_fuzzer_main.cc, which is stack frame 5.

Suspected Project: chromium-webrtc
Suspected Component: Blink>Webrtc

henrik@: Could you please take a look at this.

Thanks in advance!
Cc: ossu@chromium.org kwiberg@chromium.org
Components: -Blink>WebRTC Blink>WebRTC>Audio
+ossu, kwiberg
We have a memory leak in NetEq, triggered by fuzzer tests. The test has only been running for a few weeks, so it is not clear to me if this regressed recently or if it has been broken for a long time.

Can either of you take a look and see if this is easy to fix?

Comment 3 by ossu@chromium.org, Oct 19 2016

I have a CL on the way which will remove all manual memory management (of packets) from NetEq. Hopefully that will solve it!

Comment 4 by ossu@chromium.org, Oct 19 2016

Owner: ossu@chromium.org
I was able to reproduce this locally (had to set ASAN_OPTIONS from the report manually, but I guess that's how it's supposed to work) on master.
I was not able to reproduce it in my local branch for the CL that changes NetEq to use Packet as a value type: https://codereview.webrtc.org/2425223002/

So ... I guess I fixed it before I knew there was a problem. Lucky! :)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/a73f6c9726322021446696faf47c93f431f6104a

commit a73f6c9726322021446696faf47c93f431f6104a
Author: ossu <ossu@webrtc.org>
Date: Mon Oct 24 15:25:28 2016

NetEq now works with packets as values, rather than pointers.

PacketList is now list<Packet> instead of list<Packet*>.
Splicing the lists in NetEqImpl::InsertPacketInternal instead of
moving packets. Avoid moving the packet when doing Rfc3389Cng.
Removed PacketBuffer::DeleteFirstPacket and DeleteAllPackets.

BUG= chromium:657300 

Review-Url: https://codereview.webrtc.org/2425223002
Cr-Commit-Position: refs/heads/master@{#14747}

[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/comfort_noise.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/comfort_noise.h
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/decoder_database.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/decoder_database_unittest.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/mock/mock_packet_buffer.h
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/neteq_impl.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/packet.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/packet.h
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/packet_buffer.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/packet_buffer.h
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/packet_buffer_unittest.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/red_payload_splitter.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/red_payload_splitter_unittest.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/timestamp_scaler.cc
[modify] https://crrev.com/a73f6c9726322021446696faf47c93f431f6104a/webrtc/modules/audio_coding/neteq/timestamp_scaler_unittest.cc

Project Member

Comment 6 by ClusterFuzz, Oct 27 2016

ClusterFuzz has detected this issue as fixed in range 427617:427664.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6673791537381376

Fuzzer: libfuzzer_neteq_rtp_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  webrtc::NetEqImpl::InsertPacketInternal
  webrtc::NetEqImpl::InsertPacket
  webrtc::test::NetEqTest::Run
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=423124:423149
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_asan&range=427617:427664

Minimized Testcase (0.49 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94pK8LqxvxLhhwathU4apvZnzHaktm54XY122Hg7f7UT4EA5WWlI_R1aSJige38ih3ESuglFsnYVY4taml7s8itQ8FL8Qpan2l4L8lUWsMLwflh6Dutp851sqx4PcktIwlCLFnEHtsHRkqFC3NmCaY9HAVAIQ?testcase_id=6673791537381376

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 7 by ClusterFuzz, Oct 27 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 8 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment