654197 - nsWebSocket::Initialize uses an unanchored JSString unsafely

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Josh Matthews [:jdm]]

>2958   JSString* jsstr = JS_ValueToString(aContext, aArgv[0]);
>2959   if (!jsstr) {
>2960     return NS_ERROR_DOM_SYNTAX_ERR;
>2961   }
>2962 
>2963   size_t length;
>2964   const jschar *chars = JS_GetStringCharsAndLength(aContext, jsstr, &length);

and later on

>2972     jsstr = JS_ValueToString(aContext, aArgv[1]);
>2973     if (!jsstr) {
>2974       return NS_ERROR_DOM_SYNTAX_ERR;
>2975     }
>2976 
>2977     chars = JS_GetStringCharsAndLength(aContext, jsstr, &length);

This is unsafe. There needs to be a |js::Anchor<JSString*> str(jsstr)| added each time jsstr is assigned, to keep it from being GCed.

Comment 1

[Patrick McManus [:mcmanus]]

> This is unsafe. There needs to be a |js::Anchor<JSString*> str(jsstr)| added
> each time jsstr is assigned, to keep it from being GCed.

Thanks Josh - I'm not really very familiar with the JS API and I inherited this part of the websockets code. Just to confirm, this is what you mean? :

diff --git a/content/base/src/nsWebSocket.cpp b/content/base/src/nsWebSocket.cpp
--- a/content/base/src/nsWebSocket.cpp
+++ b/content/base/src/nsWebSocket.cpp
@@ -687,22 +687,25 @@ nsWebSocket::Initialize(nsISupports* aOw
   if (!jsstr) {
     return NS_ERROR_DOM_SYNTAX_ERR;
   }
 
+  JS::Anchor<JSString *> deleteProtector(jsstr);
   size_t length;
   const jschar *chars = JS_GetStringCharsAndLength(aContext, jsstr, &length);
   if (!chars) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   urlParam.Assign(chars, length);
+  deleteProtector.clear();
 
   if (aArgc == 2) {
     jsstr = JS_ValueToString(aContext, aArgv[1]);
     if (!jsstr) {
       return NS_ERROR_DOM_SYNTAX_ERR;
     }
 
+    deleteProtector.set(jsstr);
     chars = JS_GetStringCharsAndLength(aContext, jsstr, &length);
     if (!chars) {
       return NS_ERROR_OUT_OF_MEMORY;
     }

Comment 2

[Josh Matthews [:jdm]]

Yeah, that should work fine.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: fix jsref problem v1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 529810 [details] [diff] [review]
fix jsref problem v1

I'm not familiar enough with all the time changing js api.
(I tought stack walking during gc would just work in this case.)

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 529810 [details] [diff] [review]
fix jsref problem v1

Jason, can you sign off on this anchor usage?

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 529810 [details] [diff] [review]
fix jsref problem v1

Review of attachment 529810 [details] [diff] [review]:

Yes, this is necessary and correct. Thanks.

Comment 7

[Patrick McManus [:mcmanus]]

http://hg.mozilla.org/mozilla-central/rev/3d3beb323f72