Closed Bug 1140317 Opened 5 years ago Closed 5 years ago

Coverity: string allocated by DecompileValueGenerator sometimes isn't freed

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(2 files)

Found thanks to Coverity (https://scan.coverity.com/projects/56)
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)
(this fixes coverity issues with CID 1286715, CID 1286667, CID 1286476 and a few others)
Attached patch Use uniqueptrSplinter Review
And this does what's asked in the previous comment. Seems to work fine.
Attachment #8573869 - Flags: feedback?(jwalden+bmo)
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+
Attachment #8573814 - Flags: review?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.