The default bug view has changed. See this FAQ.

nsWebSocket::Initialize uses an unanchored JSString unsafely

RESOLVED FIXED

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: mcmanus)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
>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.
(Assignee)

Comment 1

6 years ago
> 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;
     }
(Reporter)

Comment 2

6 years ago
Yeah, that should work fine.
(Assignee)

Comment 3

6 years ago
Created attachment 529810 [details] [diff] [review]
fix jsref problem v1
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #529810 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Blocks: 640003
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.)
Attachment #529810 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 5

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

Jason, can you sign off on this anchor usage?
Attachment #529810 - Flags: review?(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.
Attachment #529810 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

6 years ago
http://hg.mozilla.org/mozilla-central/rev/3d3beb323f72
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.