Proliferate js_memcpy and PodCopy

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: cdleary, Assigned: cdleary)

Tracking

(Depends on 1 bug)

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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 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?
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 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+
Summary: Make js_memcpy for bounds checking → Proliferate js_memcpy and PodCopy
https://hg.mozilla.org/mozilla-central/rev/7736d47f8fab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 933149
You need to log in before you can comment on or make changes to this bug.