Closed Bug 1037897 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: js-ctypes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: gkw, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

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

Attachments

(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)
7123
    
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);
7125
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)
7136}

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 dteller@mozilla.com 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]
bug-1037897-ctypes-finalizer-tidy-v1.patch

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]
bug-1037897-ctypes-finalizer-tidy-v1.patch

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b50f97367682
Note: landed without sec-approval because this isn't security-sensitive. The original code was fine.
https://hg.mozilla.org/mozilla-central/rev/b50f97367682
Status: NEW → RESOLVED
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.