Proliferate js_memcpy and PodCopy

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

(Depends on: 1 bug)

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 588188 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

I think it'd be nice to have a js_memcpy(dst, src, len) that asserts that src does not fall in the range [dst, dst + len) and use it throughout the engine.

This passes locally, pushing to try.
Attachment #588188 - Flags: review?(luke)

Comment 1

5 years ago
Comment on attachment 588188 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

>+static JS_ALWAYS_INLINE void *
>+js_memcpy(void *dst, const void *src, size_t len)
>+{
>+    /* Check that src is not in [dst, dst + len). */
>+    JS_ASSERT(!(dst <= src && src < (char *) dst + len));
>+    return memcpy(dst, src, len);
>+}

I think non-overlapping-ness wants an even stronger assert, say, like the one in PodCopy:

>@@ -316,17 +324,17 @@ PodCopy(T *dst, const T *src, size_t nel
>     /* Cannot find portable word-sized abs(). */
>     JS_ASSERT_IF(dst >= src, size_t(dst - src) >= nelem);
>     JS_ASSERT_IF(src >= dst, size_t(src - dst) >= nelem);

Second, it looks like a lot (but not all) of these memcpy uses could be PodCopy instead (avoiding the sizeof(T) computation).  Could you switch them since you are touching all these lines anyway?
Created attachment 588229 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

Now with PodAssign and a bunch of PodCopy's!
Attachment #588188 - Attachment is obsolete: true
Attachment #588188 - Flags: review?(luke)
Attachment #588229 - Flags: review?(luke)

Comment 3

5 years ago
Comment on attachment 588229 [details] [diff] [review]
Make js_memcpy and use it in a bunch of places.

r+ with more PodCopy/PodAssign in TradeGuts and JSScript::NewScriptFromEmitter (if possible) and with verification that the generated asm isn't any worse.
Attachment #588229 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b36175bbda47
Target Milestone: --- → mozilla12
Summary: Make js_memcpy for bounds checking → Proliferate js_memcpy and PodCopy
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd329d75054
Target Milestone: mozilla12 → ---
Take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/7736d47f8fab
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/7736d47f8fab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 933149
You need to log in before you can comment on or make changes to this bug.