ssltunnel parseConfigFile leaks |f| and parseConfigLine leaks |authoption|

RESOLVED FIXED in Firefox 41

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: erahm, Assigned: rnath, Mentored)

Tracking

({coverity})

unspecified
mozilla41
x86_64
Linux
coverity
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [MemShrink:P3][CID 1286402][CID 1285943][lang=c++][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

|authoption| can also be leaked in |parseConfigLine| if there is an early return [1].

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/testing/mochitest/ssltunnel/ssltunnel.cpp#l1240
Summary: ssltunnel parseConfigFile leaks |f| → ssltunnel parseConfigFile leaks |f| and parseConfigLine leaks |authoption|
Whiteboard: [MemShrink][CID 1286402] → [MemShrink][CID 1286402][CID 1285943]
Not a particularly big deal since:
a) This is just a test tool and
b) Early return probably means the tool is going to quit anyway.
Whiteboard: [MemShrink][CID 1286402][CID 1285943] → [MemShrink:P3][CID 1286402][CID 1285943]
Mentor: erahm
Whiteboard: [MemShrink:P3][CID 1286402][CID 1285943] → [MemShrink:P3][CID 1286402][CID 1285943][lang=c++][good first bug]
(Assignee)

Comment 3

3 years ago
Can someone give me more info on this bug? I would like to work on it.
Ryan: in the code linked in comment 0, we're missing an `fclose(f);` right before the `return 1;` at the linked line.

Similarly, the allocation linked in comment 1 can be leaked in the early return here:
https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/testing/mochitest/ssltunnel/ssltunnel.cpp#l1255

so we'd need a `delete client_auth_option;` before the return.
(Assignee)

Comment 5

3 years ago
Created attachment 8607214 [details] [diff] [review]
Bug_1142684revA.patch

This patch stops the leaking of |f| and |authoption| from occurring.
Comment on attachment 8607214 [details] [diff] [review]
Bug_1142684revA.patch

Review of attachment 8607214 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start! There are a few other places where |authoption| needs to be cleaned up as well:
  - https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/testing/mochitest/ssltunnel/ssltunnel.cpp#l1262
  - https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/testing/mochitest/ssltunnel/ssltunnel.cpp#l1318
Attachment #8607214 - Flags: feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8607325 [details] [diff] [review]
Bug_1142684revB.patch

Updates per Eric's comments.
Comment on attachment 8607325 [details] [diff] [review]
Bug_1142684revB.patch

Review of attachment 8607325 [details] [diff] [review]:
-----------------------------------------------------------------

Ted would you mind reviewing this?
Attachment #8607325 - Flags: review?(ted)
Comment on attachment 8607325 [details] [diff] [review]
Bug_1142684revB.patch

Review of attachment 8607325 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
Attachment #8607325 - Flags: review?(ted) → review+
Assignee: nobody → nathmatics
Attachment #8607214 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8608480 [details] [diff] [review]
patch for checkin (now with commit message)

This is a final patch for this bug. I need someone to checkin.
(Assignee)

Updated

3 years ago
Keywords: coverity → checkin-needed
Keywords: coverity
Comment on attachment 8607325 [details] [diff] [review]
Bug_1142684revB.patch

Thanks for the patch, Ryan! Can you please update your commit info to use your full name? Thanks :)
Attachment #8607325 - Attachment is obsolete: true
Depends on: 1167249
https://hg.mozilla.org/mozilla-central/rev/c4bab6f234f6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.