open
https://gitlab.synchro.net/main/sbbs/-/issues/1144
**Summary**
The Synchronet JS binding layer (src/sbbs3/js_*.cpp) calls SpiderMonkey API functions that can fail (JS_DefineProperty, JS_NewObject, JS_NewStringCopyZ, JS_DefineElement, JS_NewArrayObject, JS_SetProperty, etc.) without checking the return value. On failure these functions set a pending exception on cx and return JS_FALSE / NULL. The binding code ignores both, leaving the exception silently pending while continuing execution with the unsuccessful side-effect (no property defined, NULL object pointer, missing array element, etc.).
**Scope**
Quick survey (approximate, by grep) in the current tree:
- JS_DefineProperty: 683 call sites tree-wide. ~108 inside any if, and most of those ifs gate the call rather than check its return. Actual checked-return rate is well under 10%.
- src/sbbs3/js_msgbase.cpp: 28 JS_DefineProperty calls, 0 return-value checks. - Same pattern applies to JS_NewObject, JS_NewStringCopyZ, JS_NewArrayObject, JS_DefineElement, JS_SetProperty.
Concrete instance (the case that surfaced this)
The LAZY_STRING_TRUNCSP and LAZY_STRING_TRUNCSP_NULL macros in js_msgbase.cpp:1322-1350 are textbook examples:
```
#define LAZY_STRING_TRUNCSP_NULL(PropName, PropValue, flags) \
if (name == NULL || strcmp(name, (PropName)) == 0) { \
if (name) free(name); \
if ((PropValue) != NULL && (js_str = JS_NewStringCopyZ(cx, truncsp(PropValue))) != NULL) { \
JS_DefineProperty(cx, obj, PropName, STRING_TO_JSVAL(js_str), NULL, NULL, flags); \
if (name) return JS_TRUE; \
} \
else if (name) return JS_TRUE; \
}
```
If JS_NewStringCopyZ returns NULL (OOM, allocator failure), it sets a pending exception on cx. The macro falls through to else if (name) return JS_TRUE and the resolve hook tells the engine "resolved successfully, no property defined." CallResolveOp (jsobj.cpp:4927-4938) takes the JS_TRUE at face value, nativeLookup(id) returns NULL, and js_GetPropertyHelperWithShapeInline (jsobj.cpp:5361) sets vp->setUndefined() and returns success. The script observes undefined. The exception sits on cx until something else trips on it.
The JS_DefineProperty inside that same macro is also unchecked; if it fails, the macro still returns JS_TRUE claiming success.
**Consequences**
- Under any allocation pressure, JS-visible reads can return spurious undefined (or NULL object pointers, or missing array elements) with no script-visible error at the moment of failure.
- The pending exception eventually surfaces somewhere unrelated to where it was actually produced, making postmortem debugging hard.
- This is a candidate contributor to the symptom investigated in #1143 (cold get_all_msg_headers() reads of *_NULL fields returning undefined on a real mail base). Whether OOM is the actual trigger for that specific run is open (test plan in the #1143 thread); the defect pattern documented here is separable and worth fixing on its own merits.
**Suggested fix direction**
Audit and convert call sites in src/sbbs3/js_*.cpp to check returns and propagate failure:
```
if ((js_str = JS_NewStringCopyZ(cx, ...)) == NULL)
return JS_FALSE;
if (!JS_DefineProperty(cx, obj, name, val, NULL, NULL, flags))
return JS_FALSE;
```
For resolve-hook helpers like the LAZY_* macros, "propagate failure" means returning JS_FALSE rather than swallowing it into JS_TRUE-without-define.
Sites that need to clean up local state before returning (free allocated buffers, unlock SMB handles, release private structs, etc.) need their failure paths updated to do that cleanup. So this isn't a pure mechanical sweep, but it can be batched per file.
js_msgbase.cpp is a natural first target since it's the file that surfaced this in #1143.
**References**
- #1143 - get_all_msg_headers() cold *_NULL reads via dot-access (the issue that surfaced this).
- SpiderMonkey 1.8.5 resolve-hook contract: CallResolveOp at 3rdp/src/mozjs/js-1.8.5/js/src/jsobj.cpp:4859-4947 does not check pending-exception state after the hook returns.
--- SBBSecho 3.37-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)