Possible memory leak in ReadStringCommon in CTypes.cpp.

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: arai, Assigned: noitidart, Mentored)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → 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:
```
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:
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:
Posted patch patch (obsolete) — Splinter Review
Woohoo first patch uploaded! Hi Bobby may you please review
Attachment #8775566 - Flags: review?(bobbyholley)
Attachment #8775566 - Flags: review?(bobbyholley) → review+
Aweeeesome! Thanks @bholley!!!
Posted patch patch.txt version as LF (obsolete) — Splinter Review
Fixed line endings to be LF
Attachment #8775952 - Flags: review+
Attachment #8775566 - Attachment is obsolete: true
Attachment #8775952 - Attachment is obsolete: true
fixed patch.txt version as LF, added r=bholley to title thanks @arai!
Attachment #8775960 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2cb02275182
Free dst after failed call to JS_NewUCString. r=bholley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b2cb02275182
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.