Closed Bug 1142757 Opened 5 years ago Closed 5 years ago

media/webrtc/signaling/test/mediaconduit_unittests.cpp |AudioSendAndReceive::GenerateAndReadSamples| has several leaks


(Core :: WebRTC: Signaling, defect, P5)




Tracking Status
firefox41 --- fixed
Blocking Flags:


(Reporter: erahm, Assigned: rnath, Mentored)


(Blocks 1 open bug)


(Keywords: coverity, Whiteboard: [MemShrink][CID 1285948][CID 1260198][lang=c++][good first bug])


(1 file, 3 obsolete files)

Note: this is in test code.

Coverity indicates several leaks in AudioSendAndReceive::GenerateAndReadSamples [1].

- |inbuf| is leaked
- |inFile| is leaked if |outFile| fails to open [2]
- |outFile| can be leaked if there as early return

Rank: 55
Flags: firefox-backlog+
Priority: -- → P5
Whiteboard: [MemShrink][CID 1285948][CID 1260198] → [MemShrink:P3][CID 1285948][CID 1260198]
Mentor: erahm
Whiteboard: [MemShrink:P3][CID 1285948][CID 1260198] → [MemShrink:P3][CID 1285948][CID 1260198][lang=c++][good first bug]
backlog: --- → webRTC+
Flags: firefox-backlog+
Whiteboard: [MemShrink:P3][CID 1285948][CID 1260198][lang=c++][good first bug] → [MemShrink][CID 1285948][CID 1260198][lang=c++][good first bug]
Can someone provide me more info on this bug?
Comment 0 contains the important information -- there is one memory leak and two potential file descriptor leaks in this function. Adding |free| and |fclose| calls in the appropriate place will fix them.
Attached patch Bug1142757_revA.patch (obsolete) — Splinter Review
This patch removes the leaks pointed out by Coverity - |free| was issued to deallocate |inbuf|, and |fclose| was issued to close the file stream of |inFile| and |outFile|.
Once you are ready to ask for review, you can request on the [review] page by selecting '?' in the dropdown next to "review", and selecting a reviewer. I'd be happy to handle the review for you.
Attached patch Bug1142757_revA.patch (obsolete) — Splinter Review
Attachment #8607306 - Flags: review?(docfaraday)
Comment on attachment 8607306 [details] [diff] [review]

Review of attachment 8607306 [details] [diff] [review]:

Just a couple of minor fixes.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +305,5 @@
>       return;
>     }
>     FILE* outFile = fopen( oFile.c_str(), "wb+");
> +   if(!outFile) 

Let's leave the formatting alone here; the convention is supposed to be opening-brace-on-the-same-line. Not that this file is consistent...

@@ +327,5 @@
>     {
>      if(!memcpy(audioInput.get(), inbuf, sampleLengthInBytes))
>      {
> +      free(inbuf);
> +      fclose(inFile);

No need to fclose(inFile) here, we do that unconditionally on line 322.
Attachment #8607306 - Flags: review?(docfaraday)
Attached patch Bug1142757_revB.patch (obsolete) — Splinter Review
Updates per Byron's comments
Attachment #8607347 - Flags: review?(docfaraday)
Comment on attachment 8607347 [details] [diff] [review]

Review of attachment 8607347 [details] [diff] [review]:

You didn't have to supply an interdiff, but it is appreciated. Go ahead and fold these into a single patch so the commit log is clean, and we can get this checked in.
Attachment #8607347 - Flags: review?(docfaraday) → review+
Also, make sure to put a commit message of the form "Bug 1142757: <message goes here>"
Final patch.
Attachment #8607709 - Flags: checkin?(docfaraday)
Comment on attachment 8607709 [details] [diff] [review]
Commit message added

Review of attachment 8607709 [details] [diff] [review]:

Carry forward r+. Let me push this to try.
Attachment #8607709 - Flags: review+
Comment on attachment 8607709 [details] [diff] [review]
Commit message added

In the future, you can just set the checkin-needed bug keyword. It behaves more sanely with the tools we use for marking bugs post-landing.
Attachment #8607709 - Flags: checkin?(docfaraday) → checkin+
Assignee: nobody → nathmatics
Keywords: checkin-needed
Thanks Ryan (and Ryan)!
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.