Closed Bug 1037897 Opened 6 years ago Closed 6 years ago

CID 748348: Out-of-bounds access as found by Coverity


(Core :: js-ctypes, defect)

Not set



Tracking Status
firefox33 --- fixed
firefox-esr31 --- wontfix


(Reporter: gkw, Assigned: jorendorff)


(Blocks 1 open bug)


(Keywords: coverity, sec-other, Whiteboard: [adv-main33-])


(1 file)

+++ This bug was initially created as a clone of Bug #1037890 +++

Coverity analysis of source code in js/src has found an Out-of-bounds access.

7117  int savedErrno = errno;
7118  errno = 0;
7119#if defined(XP_WIN)
7120  int32_t savedLastError = GetLastError();
7121  SetLastError(0);
7122#endif // defined(XP_WIN)
1. address_of: Taking address with &p->cargs yields a singleton pointer.
CID 748348 (#1 of 1): Out-of-bounds access (ARRAY_VS_SINGLETON)2. callee_ptr_arith: Passing &p->cargs to function ffi_call which uses it as an array. This might corrupt or misinterpret adjacent memory locations. [show details]
7124  ffi_call(&p->CIF, FFI_FN(p->code), p->rvalue, &p->cargs);
7126  if (errnoStatus) {
7127    *errnoStatus = errno;
7128  }
7129  errno = savedErrno;
7130#if defined(XP_WIN)
7131  if (lastErrorStatus) {
7132    *lastErrorStatus = GetLastError();
7133  }
7134  SetLastError(savedLastError);
7135#endif // defined(XP_WIN)

in file js/src/ctypes/CTypes.cpp .

Jan, any thoughts on how to move forward here? (not sure how bad this is, so setting s-s first.)
Flags: needinfo?(jdemooij)
Group: javascript-core-security
Forwarding to Jason as he owns js-ctypes and I'm not very familiar with it.
Component: JavaScript Engine → js-ctypes
Flags: needinfo?(jdemooij) → needinfo?(jorendorff)
This one is probably fine. I'll look at it tomorrow.
This dates back to the original patch for ctypes finalizers in bug 720771, and the code is fine as written. ffi_call expects an array of length 1.

However just to be totally sure the compiler doesn't see undefined behavior and go on to prove black is white and get killed at the next zebra crossing---and to make Coverity happy---there's a simple local stylistic fix.
Flags: needinfo?(jorendorff)
Yoric is probably the best person to say whether this change is correct, but Bugzilla literally will not let me select as reviewer.

The existing tests can be run with:
  ./mach test toolkit/components/ctypes/tests/unit/test_finalizer.js
and they still pass with the change.
Assignee: nobody → jorendorff
Attachment #8457044 - Flags: review?(jdemooij)
add non-sg members to cc first, then mark the review
Attachment #8457044 - Flags: review?(dteller)
Comment on attachment 8457044 [details] [diff] [review]

Review of attachment 8457044 [details] [diff] [review]:

Looks good to me.

::: js/src/ctypes/CTypes.cpp
@@ +6905,5 @@
>      return false;  // We have called |dispose| or |forget| already.
>    }
>    RootedObject ctype(cx, GetCType(cx, obj));
> +  return ConvertToJS(cx, ctype, /*parent*/NullPtr(), p->cargs, false, true, aResult);

Whitespace change?
Attachment #8457044 - Flags: review?(dteller) → review+
Comment on attachment 8457044 [details] [diff] [review]

Review of attachment 8457044 [details] [diff] [review]:

Clearing review flag since Yoric reviewed it already.
Attachment #8457044 - Flags: review?(jdemooij)
The whitespace change was intentional---just a trivial style fix, in passing.
Note: landed without sec-approval because this isn't security-sensitive. The original code was fine.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: sec-other
Whiteboard: [adv-main33-]
Marking [qe-verify-], due to Coverity code audit, no test case or STR.
Flags: qe-verify-
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.