If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Assertion of non-null-terminated string on returning a ctypes array.readString()

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Mardak, Assigned: dwitte@gmail.com)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
###!!! ASSERTION: nsTDependentString must wrap only null-terminated strings: 'mData[mLength] == 0', file ../../dist/include/nsTDependentString.h, line 67
nsDependentString::AssertValid()+0x000000CF [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x003E2C0F]
nsDependentString::nsDependentString(unsigned short const*, unsigned int)+0x00000033 [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x00025909]
XPCConvert::JSData2Native(XPCCallContext&, void*, long, nsXPTType const&, int, nsID const*, unsigned int*)+0x000010AD [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x00DBB2A3]
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*)+0x00001AA6 [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x00DDA98A]
nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*)+0x00000061 [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x00DD129F]
PrepareAndDispatch(nsXPTCStubBase*, unsigned int, unsigned int*)+0x0000027A [/Users/Ed/sync-trunk/objdir/dist/bin/XUL +0x01480024]

The code triggering the assertion is the call to decrypt():
http://hg.mozilla.org/services/fx-sync/file/0fa59ea429d7/services/sync/tests/unit/test_crypto_crypt.js#l34

The assertion is after decrypt() returns outputBuffer.readString():
http://hg.mozilla.org/services/fx-sync/file/0fa59ea429d7/services/crypto/WeaveCrypto.js#l515

In this case the output buffer has this value:
ctypes.unsigned_char.array(0)([])

And the value, "" (empty string), is usable from within decrypt().

dwitte provides these links of 1) ctypes code that generates the string 2) xpconnect code to convert the string:
http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#5526
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcconvert.cpp#879

dwitte: so xpconnect calls JS_GetStringChars before that line, which does not guarantee nulltermination
dwitte: it's calling JS_GetStringChars then wrapping it in an nsDependentString for passing to CopyUTF16toUTF8


For now, WeaveCrypto.js can work around this issue by doing:
return "" + outputBuffer.readString() + "";
(Reporter)

Updated

7 years ago
Blocks: 573842
(Assignee)

Comment 1

7 years ago
Created attachment 459129 [details] [diff] [review]
patch v1

Fixes JS_GetStringChars usage where appropriate. I tried to avoid using JS_GetStringCharsZ where I could, since it might allocate. JS_GetStringBytes apparently nullterminates.

Note that the !useAllocator case around #866 does get hit a bunch on startup -- so we can't remove it or assert, I think. (The caller must assume nulltermination, I guess!)

This also removes an overallocation in ctypes, which AFAICT is unnecessary.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Attachment #459129 - Flags: review?(mrbkap)
(Assignee)

Comment 2

7 years ago
Created attachment 459158 [details] [diff] [review]
patch v2

And this one actually works.
Attachment #459129 - Attachment is obsolete: true
Attachment #459158 - Flags: review?(mrbkap)
Attachment #459129 - Flags: review?(mrbkap)
Comment on attachment 459158 [details] [diff] [review]
patch v2

>diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp
>     jschar* dst =
>-      static_cast<jschar*>(JS_malloc(cx, (dstlen + 1) * sizeof(jschar)));
>+      static_cast<jschar*>(JS_malloc(cx, dstlen * sizeof(jschar)));
>     if (!dst)
>       return JS_FALSE;
> 
>     ASSERT_OK(js_InflateUTF8StringToBuffer(cx, bytes, length, dst, &dstlen));
> 
>     result = JS_NewUCString(cx, dst, dstlen);

Seems like this manual malloc + NewString could be a JS_NewUCStringCopyN and save some code complexity.

>diff --git a/js/src/xpconnect/src/xpcconvert.cpp b/js/src/xpconnect/src/xpcconvert.cpp
>     case nsXPTType::T_WCHAR  :
>         {
>-            jschar* chars=nsnull;
>+            const jschar* chars=nsnull;
>             JSString* str;
>             if(!(str = JS_ValueToString(cx, s))||
>+               (JS_GetStringLength(str) == 0)||

Huh, is this really an existing use of undefined memory in the str->length() == 0 case? I guess if we assume null termination, then we allow it... I think this is fine, though we should be on the lookout for web pages/extensions that try to pass empty strings as characters and manage to work for now, since this is a change in behavior.
Attachment #459158 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 4

7 years ago
Created attachment 459593 [details] [diff] [review]
patch v3

Handles the length == 0 case like before.

Requesting approval -- xpconnect correctness is pretty important, and this is biting us in Sync.
Attachment #459158 - Attachment is obsolete: true
Attachment #459593 - Flags: review+
Attachment #459593 - Flags: approval2.0?

Updated

7 years ago
Attachment #459593 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 5

7 years ago
http://hg.mozilla.org/tracemonkey/rev/5445b352e07d
Whiteboard: fixed-in-tracemonkey
Depends on: 582521

Comment 6

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