Skip to content

Fix the string corruption bug #428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Fix the string corruption bug #428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f3039b
fix: heap use-after-free for `JS::UniqueChars`
Xmader Sep 4, 2024
935e9e9
fix: the string corruption bug
Xmader Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/PyObjectProxyHandler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ bool PyObjectProxyHandler::handleGetOwnPropertyDescriptor(JSContext *cx, JS::Han
JS::MutableHandle<mozilla::Maybe<JS::PropertyDescriptor>> desc, PyObject *item) {
// see if we're calling a function
if (id.isString()) {
JS::RootedString idString(cx, id.toString());
const char *methodName = JS_EncodeStringToUTF8(cx, idString).get();
JS::UniqueChars idString = JS_EncodeStringToUTF8(cx, JS::RootedString(cx, id.toString()));
Copy link
Collaborator

@philippedistributive philippedistributive Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dos this fix a heap use after free error?

Copy link
Member Author

@Xmader Xmader Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it also fixes one, but not the main one that's due to the GC moving the string buffer around.
This one is about that the JS::UniqueChars obtained by JS_EncodeStringToUTF8(cx, idString) gets free-ed immediately after this line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, as expected, not the main one

const char *methodName = idString.get();

if (!strcmp(methodName, "toString") || !strcmp(methodName, "toLocaleString") || !strcmp(methodName, "valueOf")) {
JS::RootedObject objectPrototype(cx);
if (!JS_GetClassPrototype(cx, JSProto_Object, &objectPrototype)) {
Expand Down
43 changes: 29 additions & 14 deletions src/modules/pythonmonkey/pythonmonkey.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,25 +48,39 @@

JS::PersistentRootedObject jsFunctionRegistry;

/**
* @brief During a GC, string buffers may have moved, so we need to re-point our JSStringProxies
* The char buffer pointer obtained by previous `JS::Get{Latin1,TwoByte}LinearStringChars` calls remains valid only as long as no GC occurs.
*/
void updateCharBufferPointers() {
if (_Py_IsFinalizing()) {
return; // do not move char pointers around if python is finalizing
}

JS::AutoCheckCannotGC nogc;
for (const JSStringProxy *jsStringProxy: jsStringProxies) {
JSLinearString *str = JS_ASSERT_STRING_IS_LINEAR(jsStringProxy->jsString->toString());
void *updatedCharBufPtr; // pointer to the moved char buffer after a GC
if (JS::LinearStringHasLatin1Chars(str)) {
updatedCharBufPtr = (void *)JS::GetLatin1LinearStringChars(nogc, str);
} else { // utf16 / ucs2 string
updatedCharBufPtr = (void *)JS::GetTwoByteLinearStringChars(nogc, str);
}
((PyUnicodeObject *)(jsStringProxy))->data.any = updatedCharBufPtr;
}
}

void pythonmonkeyGCCallback(JSContext *cx, JSGCStatus status, JS::GCReason reason, void *data) {
if (status == JSGCStatus::JSGC_END) {
JS::ClearKeptObjects(GLOBAL_CX);
while (JOB_QUEUE->runFinalizationRegistryCallbacks(GLOBAL_CX));
updateCharBufferPointers();
}
}

if (_Py_IsFinalizing()) {
return; // do not move char pointers around if python is finalizing
}

JS::AutoCheckCannotGC nogc;
for (const JSStringProxy *jsStringProxy: jsStringProxies) { // char buffers may have moved, so we need to re-point our JSStringProxies
JSLinearString *str = (JSLinearString *)(jsStringProxy->jsString->toString()); // jsString is guaranteed to be linear
if (JS::LinearStringHasLatin1Chars(str)) {
(((PyUnicodeObject *)(jsStringProxy))->data.any) = (void *)JS::GetLatin1LinearStringChars(nogc, str);
}
else { // utf16 / ucs2 string
(((PyUnicodeObject *)(jsStringProxy))->data.any) = (void *)JS::GetTwoByteLinearStringChars(nogc, str);
}
}
void nurseryCollectionCallback(JSContext *cx, JS::GCNurseryProgress progress, JS::GCReason reason, void *data) {
if (progress == JS::GCNurseryProgress::GC_NURSERY_COLLECTION_END) {
updateCharBufferPointers();
}
}

Expand Down Expand Up @@ -565,6 +579,7 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void)
JS_SetGCParameter(GLOBAL_CX, JSGC_MAX_BYTES, (uint32_t)-1);

JS_SetGCCallback(GLOBAL_CX, pythonmonkeyGCCallback, NULL);
JS::AddGCNurseryCollectionCallback(GLOBAL_CX, nurseryCollectionCallback, NULL);

JS::RealmCreationOptions creationOptions = JS::RealmCreationOptions();
JS::RealmBehaviors behaviours = JS::RealmBehaviors();
Expand Down
Loading