1140317 - Coverity: string allocated by DecompileValueGenerator sometimes isn't freed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Found thanks to Coverity (https://scan.coverity.com/projects/56)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Fix a few resource leaks related to DecompileValueGenerator

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?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(this fixes coverity issues with CID 1286715, CID 1286667, CID 1286476 and a few others)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Use uniqueptr

And this does what's asked in the previous comment. Seems to work fine.

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/f257cfba6686

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f257cfba6686
https://hg.mozilla.org/mozilla-central/rev/ff5ddf743796
https://hg.mozilla.org/mozilla-central/rev/3f28ce99ab61