Last Comment Bug 654197 - nsWebSocket::Initialize uses an unanchored JSString unsafely
: nsWebSocket::Initialize uses an unanchored JSString unsafely
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks: 640003
  Show dependency treegraph
 
Reported: 2011-05-02 12:14 PDT by Josh Matthews [:jdm]
Modified: 2011-05-06 13:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix jsref problem v1 (1.23 KB, patch)
2011-05-03 12:57 PDT, Patrick McManus [:mcmanus]
jorendorff: review+
Details | Diff | Review

Description Josh Matthews [:jdm] 2011-05-02 12:14:18 PDT
>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] 2011-05-03 12:16:14 PDT
> 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] 2011-05-03 12:43:11 PDT
Yeah, that should work fine.
Comment 3 Patrick McManus [:mcmanus] 2011-05-03 12:57:51 PDT
Created attachment 529810 [details] [diff] [review]
fix jsref problem v1
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-05-04 03:30:44 PDT
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] 2011-05-04 05:05:48 PDT
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] 2011-05-04 07:22:24 PDT
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] 2011-05-06 13:24:51 PDT
http://hg.mozilla.org/mozilla-central/rev/3d3beb323f72

Note You need to log in before you can comment on or make changes to this bug.