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

Status: RESOLVED FIXED

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

Description

[Eric Rahm [:erahm]]

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

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/media/webrtc/signaling/test/mediaconduit_unittests.cpp#l286
[2] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/media/webrtc/signaling/test/mediaconduit_unittests.cpp#l310

Comment 1

[Ryan Nath [:rnath]]

Can someone provide me more info on this bug?

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Ryan Nath [:rnath]]

Attachment: Bug1142757_revA.patch

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|.

Comment 4

[Byron Campen [:bwc]]

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.

Comment 5

[Ryan Nath [:rnath]]

Attachment: Bug1142757_revA.patch

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8607306 [details] [diff] [review]
Bug1142757_revA.patch

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.

Comment 7

[Ryan Nath [:rnath]]

Attachment: Bug1142757_revB.patch

Updates per Byron's comments

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8607347 [details] [diff] [review]
Bug1142757_revB.patch

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.

Comment 9

[Byron Campen [:bwc]]

Also, make sure to put a commit message of the form "Bug 1142757: <message goes here>"

Comment 10

[Ryan Nath [:rnath]]

Attachment: Commit message added

Final patch.

Comment 11

[Byron Campen [:bwc]]

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.

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a862e15d5427

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7309844868d

Comment 15

[Byron Campen [:bwc]]

Thanks Ryan (and Ryan)!

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a7309844868d