1288907 - Possible memory leak in ReadStringCommon in CTypes.cpp.

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Tooru Fujisawa [:arai]]

In ReadStringCommon, JS_NewUCString is called with an allocated buffer.

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/js/src/ctypes/CTypes.cpp#7892
> static bool
> ReadStringCommon(JSContext* cx, InflateUTF8Method inflateUTF8, unsigned argc,
>                  Value* vp, const char* funName)
> {
> ...
>     char16_t* dst = inflateUTF8(cx, JS::UTF8Chars(bytes, length), &length).get();
>     if (!dst)
>       return false;
> 
>     result = JS_NewUCString(cx, dst, length);
>     break;
>   }
> ...
>   }
> 
>   if (!result)
>     return false;


There inflateUTF8 is either JS::UTF8CharsToNewTwoByteCharsZ or JS::LossyUTF8CharsToNewTwoByteCharsZ.

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/js/src/ctypes/CTypes.cpp#7919
>   return ReadStringCommon(cx, JS::UTF8CharsToNewTwoByteCharsZ, argc, vp,
>                           "CData.prototype.readString");
> ...
>   return ReadStringCommon(cx, JS::UTF8CharsToNewTwoByteCharsZ, argc, vp,
>                           "CDataFinalizer.prototype.readString");
> ...
>   return ReadStringCommon(cx, JS::LossyUTF8CharsToNewTwoByteCharsZ, argc, vp,
>                           "CData.prototype.readStringReplaceMalformed");


But JS_NewUCString doesn't take ownership of the passed buffer when it fails, and the passed buffer is kept allocated forever.

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/js/src/jsapi.h#4551
>  * NB: JS_NewUCString takes ownership of bytes on success, avoiding a copy;
>  * but on error (signified by null return), it leaves chars owned by the
>  * caller. So the caller must free bytes in the error case, if it has no use
>  * for them. In contrast, all the JS_New*StringCopy* functions do not take
>  * ownership of the character memory passed to them -- they copy it.


So, ReadStringCommon should free `dst` buffer when JS_NewUCString fails and returns nullptr.

Comment 1

[noitidart]

Code ready, trying to make it a patch now, this is my diff:

```
@@ -7890,6 +7890,10 @@ ReadStringCommon(JSContext* cx, InflateUTF8Method inflateUTF8, unsigned argc,
       return false;
 
     result = JS_NewUCString(cx, dst, length);
+    if (!result) {
+      js_free(dst);
+    }
+
     break;
   }
   case TYPE_int16_t:
```

Comment 2

[noitidart]

I forgot to `return nullptr` here it is fixed:

		@@ -7890,6 +7890,11 @@ ReadStringCommon(JSContext* cx, InflateUTF8Method inflateUTF8, unsigned argc,
		       return false;
		 
		     result = JS_NewUCString(cx, dst, length);
		+    if (!result) {
		+      js_free(dst);
		+      return nullptr;
		+    }
		+
		     break;
		   }
		   case TYPE_int16_t:

Comment 3

[noitidart]

Correction, return type is bool:

> diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp
> index bfa5b8f..40ec8a3 100644
> --- a/js/src/ctypes/CTypes.cpp
> +++ b/js/src/ctypes/CTypes.cpp
> @@ -7890,6 +7890,11 @@ ReadStringCommon(JSContext* cx, InflateUTF8Method inflateUTF8, unsigned argc,
>        return false;
>  
>      result = JS_NewUCString(cx, dst, length);
> +    if (!result) {
> +      js_free(dst);
> +      return false;
> +    }
> +
>      break;
>    }
>    case TYPE_int16_t:

Comment 4

[noitidart]

Attachment: patch

Woohoo first patch uploaded! Hi Bobby may you please review

Comment 5

[noitidart]

Aweeeesome! Thanks @bholley!!!

Comment 6

[noitidart]

Attachment: patch.txt version as LF

Fixed line endings to be LF

Comment 7

[noitidart]

Attachment: patch.txt version as LF and r=bholley

fixed patch.txt version as LF, added r=bholley to title thanks @arai!

Comment 8

[Tooru Fujisawa [:arai]]

here's try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=258fb01272a7

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2cb02275182
Free dst after failed call to JS_NewUCString. r=bholley

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2cb02275182