Closed
Bug 1140317
Opened 10 years ago
Closed 9 years ago
Coverity: string allocated by DecompileValueGenerator sometimes isn't freed
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
(Blocks 1 open bug)
Details
(Keywords: coverity)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Found thanks to Coverity (https://scan.coverity.com/projects/56)
Assignee | ||
Comment 1•10 years ago
|
||
Waldo, do you prefer manual js_free, or using a UniquePtr for all these cases where the char* bytes was allocated without being freed? How could we enforce auto-free rather than this? Could DecompileValueGenerator return a UniquePtr instead?
Attachment #8573814 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•10 years ago
|
||
(this fixes coverity issues with CID 1286715, CID 1286667, CID 1286476 and a few others)
Assignee | ||
Comment 3•10 years ago
|
||
And this does what's asked in the previous comment. Seems to work fine.
Attachment #8573869 -
Flags: feedback?(jwalden+bmo)
Comment 4•10 years ago
|
||
Comment on attachment 8573869 [details] [diff] [review] Use uniqueptr Review of attachment 8573869 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, UniquePtr is better than fixing up raw uses manually, for sure. ::: js/src/builtin/Object.cpp @@ +674,5 @@ > > if (!args[0].isObjectOrNull()) { > RootedValue v(cx, args[0]); > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + v, NullPtr()); UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, v, NullPtr()); is a more readable way to write this, I think. ::: js/src/jsarray.cpp @@ +3587,5 @@ > for (unsigned i = 0; i < args.length(); i++) { > RootedValue arg(cx, args[i]); > > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + arg, NullPtr()); Again, UniquePtr... = DecompileValueGenerator(...); ::: js/src/jscntxt.cpp @@ +806,2 @@ > bool ok; > + UniquePtr<char[], JS::FreePolicy> bytes(nullptr); No need for (nullptr) here. I'd do the UP = on one line, DVG(...) on the next if it were me, consistent with the other comments I've made here. @@ +835,5 @@ > void > js::ReportMissingArg(JSContext *cx, HandleValue v, unsigned arg) > { > char argbuf[11]; > + UniquePtr<char[], JS::FreePolicy> bytes(nullptr); Again no need for (nullptr). @@ +855,5 @@ > js::ReportValueErrorFlags(JSContext *cx, unsigned flags, const unsigned errorNumber, > int spindex, HandleValue v, HandleString fallback, > const char *arg1, const char *arg2) > { > + UniquePtr<char[], JS::FreePolicy> bytes(nullptr); . ::: js/src/jsobj.cpp @@ +94,5 @@ > { > if (v.isPrimitive()) { > RootedValue value(cx, v); > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + value, NullPtr()); Same split-across-lines thing. @@ +250,5 @@ > > HandleValue v = args[0]; > if (!v.isObject()) { > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + v, NullPtr()); . @@ +286,5 @@ > > /* 8.10.5 step 1 */ > if (v.isPrimitive()) { > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + v, NullPtr()); . ::: js/src/jsopcode.cpp @@ +1818,5 @@ > > return ed.getOutput(res); > } > > +typedef UniquePtr<char[], JS::FreePolicy> UniquePtrChars; mozilla:: ::: js/src/jsweakmap.cpp @@ +388,5 @@ > MOZ_ASSERT(IsWeakMap(args.thisv())); > > if (!args.get(0).isObject()) { > + UniquePtr<char[], JS::FreePolicy> bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, > + args.get(0), NullPtr()); Same.
Attachment #8573869 -
Flags: feedback?(jwalden+bmo) → review+
Updated•10 years ago
|
Attachment #8573814 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•9 years ago
|
||
Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/f257cfba6686
Assignee | ||
Comment 6•9 years ago
|
||
Two follow up with more |using mozilla::UniquePtr;| because UNIFIED BUILDS https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5ddf743796 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f28ce99ab61
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f257cfba6686 https://hg.mozilla.org/mozilla-central/rev/ff5ddf743796 https://hg.mozilla.org/mozilla-central/rev/3f28ce99ab61
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•