Closed
Bug 1216970
Opened 8 years ago
Closed 8 years ago
ProfilerImpl::GetStacktrace passes out pointers to going-to-be-freed memory
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: froydnj, Assigned: kanru)
References
Details
(Whiteboard: [adv-main44-])
Attachments
(1 file)
982 bytes,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
ProfilerImpl::GetStacktrace has: nsTArray<nsCString> trace; nsAutoArrayPtr<char> output(new char[BACKTRACE_BUFFER_SIZE]); profiler_get_backtrace_noalloc(output, BACKTRACE_BUFFER_SIZE); for (const char* p = output; *p; p += strlen(p) + 1) { trace.AppendElement(nsDependentCString(p)); } return trace; The line: trace.AppendElement(nsDependentCString(p)); seems bogus, as nsDependentCString depends on the memory it references outliving the string itself. But in this case, the memory it's referencing is going to be freed when GetStacktrace returns by nsAutoArrayPtr's destructor. (It's possible there's some tricky stuff going on under the hood with how strings get returned in this case, but that seems unlikely.) Marking as s-s for UAF. kanru, can you fix this?
Flags: needinfo?
![]() |
Reporter | |
Comment 1•8 years ago
|
||
Really ni?'ing kanru this time...Kan-Ru, can you fix this?
Flags: needinfo? → needinfo?(kchen)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kchen
Flags: needinfo?(kchen)
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #0) > ProfilerImpl::GetStacktrace has: > > nsTArray<nsCString> trace; > nsAutoArrayPtr<char> output(new char[BACKTRACE_BUFFER_SIZE]); > > profiler_get_backtrace_noalloc(output, BACKTRACE_BUFFER_SIZE); > for (const char* p = output; *p; p += strlen(p) + 1) { > trace.AppendElement(nsDependentCString(p)); > } Since we are appending to a nsTArray<nsCString>, I think the nsCString constructor should always copy the data from the nsDependentCString instead of move it. My compiled result with clang++-3.5 does invoke the nsTString_CharT(const self_type& aStr) constructor and copies the data. So it doesn't look like a UAF to me. Do you think this might not be true in some toolchain combination?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nfroyd)
![]() |
Reporter | |
Comment 3•8 years ago
|
||
Ah, indeed, copying an nsDependentCString does copy the underlying data. So there's no bug here...currently. Move construction would make this code dodgy, though, and it's arguably not obvious what's going on if one appeals to the compiled code. ;) WDYT about rewriting the loop body like so: trace.AppendElement()->Assign(p, strlen(p)); I think that makes it more obvious that the copying is taking place.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3) > Ah, indeed, copying an nsDependentCString does copy the underlying data. So > there's no bug here...currently. Move construction would make this code > dodgy, though, and it's arguably not obvious what's going on if one appeals > to the compiled code. ;) I have to admit that the nsStringAPI code is somewhat hard to trace :) > WDYT about rewriting the loop body like so: > > trace.AppendElement()->Assign(p, strlen(p)); > > I think that makes it more obvious that the copying is taking place. Works for me. Could I make that change and r=you?
![]() |
Reporter | |
Comment 5•8 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4) > (In reply to Nathan Froyd [:froydnj] from comment #3) > > Ah, indeed, copying an nsDependentCString does copy the underlying data. So > > there's no bug here...currently. Move construction would make this code > > dodgy, though, and it's arguably not obvious what's going on if one appeals > > to the compiled code. ;) > > I have to admit that the nsStringAPI code is somewhat hard to trace :) That's fair! It took me a while tracing through it to figure out what's going on, and I'm supposed to own the code! :) > > WDYT about rewriting the loop body like so: > > > > trace.AppendElement()->Assign(p, strlen(p)); > > > > I think that makes it more obvious that the copying is taking place. > > Works for me. Could I make that change and r=you? Sure, thanks. Either call Assign or Append, depending on which you think is clearer.
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/abaedc34e7e9
https://hg.mozilla.org/mozilla-central/rev/abaedc34e7e9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•8 years ago
|
Group: core-security → core-security-release
Comment 8•8 years ago
|
||
This shouldn't have been checked in without a security rating and sec-approval+ unless it is trunk only. Can you please set sec-approval? on the patch and fill out the template?
Flags: needinfo?(kchen)
Updated•8 years ago
|
Whiteboard: [adv-main44+]
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8709807 -
Flags: review+
Assignee | ||
Comment 10•8 years ago
|
||
As indicated in comment 2 and comment 3, this is not really an security issue. I'm inclined to transform this to a normal bug. What do you think?
Flags: needinfo?(kchen) → needinfo?(abillings)
Comment 11•8 years ago
|
||
I agree on reading it more closely.
Flags: needinfo?(abillings)
Whiteboard: [adv-main44+]
Updated•8 years ago
|
Whiteboard: [adv-main44-]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•