Closed
Bug 1142757
Opened 9 years ago
Closed 9 years ago
media/webrtc/signaling/test/mediaconduit_unittests.cpp |AudioSendAndReceive::GenerateAndReadSamples| has several leaks
Categories
(Core :: WebRTC: Signaling, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: erahm, Assigned: rnath, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: [MemShrink][CID 1285948][CID 1260198][lang=c++][good first bug])
Attachments
(1 file, 3 obsolete files)
2.35 KB,
patch
|
bwc
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
Rank: 55
Flags: firefox-backlog+
Priority: -- → P5
Updated•9 years ago
|
Whiteboard: [MemShrink][CID 1285948][CID 1260198] → [MemShrink:P3][CID 1285948][CID 1260198]
Reporter | ||
Updated•9 years ago
|
Mentor: erahm
Whiteboard: [MemShrink:P3][CID 1285948][CID 1260198] → [MemShrink:P3][CID 1285948][CID 1260198][lang=c++][good first bug]
Updated•9 years ago
|
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]
Assignee | ||
Comment 1•9 years ago
|
||
Can someone provide me more info on this bug?
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8607306 -
Flags: review?(docfaraday)
Comment 6•9 years ago
|
||
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.
Attachment #8607306 -
Flags: review?(docfaraday)
Assignee | ||
Comment 7•9 years ago
|
||
Updates per Byron's comments
Attachment #8607347 -
Flags: review?(docfaraday)
Comment 8•9 years ago
|
||
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.
Attachment #8607347 -
Flags: review?(docfaraday) → review+
Comment 9•9 years ago
|
||
Also, make sure to put a commit message of the form "Bug 1142757: <message goes here>"
Comment 11•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8607266 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8607306 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8607347 -
Attachment is obsolete: true
Comment 13•9 years ago
|
||
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+
Updated•9 years ago
|
Assignee: nobody → nathmatics
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7309844868d
Keywords: checkin-needed
Comment 15•9 years ago
|
||
Thanks Ryan (and Ryan)!
https://hg.mozilla.org/mozilla-central/rev/a7309844868d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•