forked from svaarala/duktape
-
Notifications
You must be signed in to change notification settings - Fork 0
/
Copy pathissue-4df1a3df6fcfc88b7515221dd083243df8747cb0.yaml
201 lines (172 loc) · 6.3 KB
/
issue-4df1a3df6fcfc88b7515221dd083243df8747cb0.yaml
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
--- !ditz.rubyforge.org,2008-03-06/issue
title: DUK_TVAL_SET_TVAL corruption in FreeBSD + clang
desc: |-
To reproduce:
* Compile in FreeBSD with clang 3.3:
$ clang -v
FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610
Target: x86_64-unknown-freebsd10.0
Thread model: posix
* Additional options: -m32, DUK_PROFILE=101
* Hand tune duk_features.h to enable all log levels
* The problem persists even when -fstrict-aliasing is NOT given, but
disappears if -Os is changed to -O0.
Incorrect behavior happens in multiple places (not sure if this is a clang/llvm
bug or incorrect assumptions in Duktape aliasing related code).
In duk_push_tval(), after DUK_TVAL_SET_TVAL(tv_slot, tv) has been called, the
memory dump of the two regions are (first byte is from 'tv', second is from
'tv_slot')::
60 60
32 32
81 81
28 28
00 00
00 00
f5 fd <==
ff ff
As can be seen, the second to last byte has been changed from F5 to FD, which
causes all sorts of problems. One possible culprit may be the DUK_TVAL_SET_TVAL()
macro itself, which simply uses *(dst) = *(src) for a duk_double_union - perhaps
this is non-portable. The problem persist even when replaced by a memmove() or
by (dst)->d = (src)->d.
Copying the values byte by byte using the 'uc' part of the union fixes this
particular problem::
#define DUK_TVAL_SET_TVAL(v,x) do { \
volatile int _i; \
for (_i = 0; _i < sizeof(duk_tval); _i++) { \
((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \
} \
} while (0)
Note that if you remove "volatile", the fix works in most call sites but not
all! This suggests clang/llvm converts the hand-coded byte copy to some
intrinsic which fails to honor the aliasing rules for char pointers.
Another problem seems to happen in duk_hobject_props.c:realloc_props, at the
line:
new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i);
The line copies a duk_propvalue to another using a plain assignment. Debug
printing the results shows that the values get corrupted in the copy the same
way as in DUK_TVAL_SET_TVAL. Some values copy OK (first is source, second is
target):
00 00
00 00
00 00
00 00
00 00
00 00
f8 f8
7f 7f
00 00
00 00
00 00
00 00
00 00
00 00
f0 f0
7f 7f
while others get corrupted again at their 7th byte (little endian memory layout).
In more concrete terms, 0x08 gets OR'd to the 7th byte. Considering the double
as a 64-bit big endian value, 0x0008'0000'0000'0000 gets OR'd to the value:
2c 2c
cb cb
ff ff
ff ff
00 00
00 00
f1 f9 <==
ff ff
10 10
49 49
81 81
28 28
00 00
00 00
f6 fe <==
ff ff
The bit in question is the highest bit of the fractional part. This can be
similarly fixed by writing a manual byte-by-byte copy, again with a volatile
loop counter.
type: :bugfix
component: duk
release:
reporter: sva <[email protected]>
status: :unstarted
disposition:
creation_time: 2013-12-10 01:28:45.479389 Z
references: []
id: 4df1a3df6fcfc88b7515221dd083243df8747cb0
log_events:
- - 2013-12-10 01:28:45.647113 Z
- sva <[email protected]>
- created
- ""
- - 2013-12-10 01:30:44.318909 Z
- sva <[email protected]>
- commented
- |-
Below is a diff of the two fixes mentioned in the description, applied to
the all-in-one source, which together fix the crashes in FreeBSD + clang
3.3. The diff is not directly applicable because it is a bit hacky, but
illustrates the idea of the fix.
Removing the 'volatile' for either of the loop variables breaks the result.
$ diff -u duktape-orig.c duktape-fixed.c
--- duktape-orig.c 2013-12-10 04:28:08.000000000 +0200
+++ duktape-fixed.c 2013-12-10 04:28:15.000000000 +0200
@@ -5832,7 +5832,12 @@
#define DUK_TVAL_SET_BUFFER(v,h) DUK__TVAL_SET_TAGGEDPOINTER((v),(h),DUK_TAG_BUFFER)
#define DUK_TVAL_SET_POINTER(v,p) DUK__TVAL_SET_TAGGEDPOINTER((v),(p),DUK_TAG_POINTER)
-#define DUK_TVAL_SET_TVAL(v,x) do { *(v) = *(x); } while (0)
+#define DUK_TVAL_SET_TVAL(v,x) do { \
+ volatile int _i; \
+ for (_i = 0; _i < 8; _i++) { \
+ ((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \
+ } \
+} while(0)
/* getters */
#define DUK_TVAL_GET_BOOLEAN(v) ((int) (v)->us[DUK_DBL_IDX_US1])
@@ -31536,7 +31541,15 @@
new_e_pv != NULL && new_e_f != NULL);
new_e_k[new_e_used] = key;
+#if 0
new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i);
+#endif
+ {
+ unsigned char *p_dst = (unsigned char *) &new_e_pv[new_e_used];
+ unsigned char *p_src = (unsigned char *) DUK_HOBJECT_E_GET_VALUE_PTR(obj, i);
+ volatile int _i;
+ for (_i = 0; _i < 8; _i++) { p_dst[_i] = p_src[_i]; }
+ }
new_e_f[new_e_used] = DUK_HOBJECT_E_GET_FLAGS(obj, i);
new_e_used++;
}
- - 2013-12-10 10:00:44.425108 Z
- sva <[email protected]>
- commented
- "See: misc/clang_aliasing.c"
- - 2013-12-10 18:19:25.100543 Z
- sva <[email protected]>
- commented
- |-
The root cause is the fact the clang ends up using a floating point load and
store to do the structured union assignment. A load+store sequence on X86
may convert a signaling NaN into a quiet NaN, which happens by setting the
highest bit of the mantissa. See, for instance:
http://caml.inria.fr/mantis/view.php?id=5038
This may be impossible to catch at compile time, so perhaps add a few self
tests and/or add an assert to DUK_TVAL_SET_TVAL. There is no telling what
other code might be affected though.
- - 2013-12-10 21:35:38.936057 Z
- sva <[email protected]>
- assigned to release v0.9 from v0.8
- ""
- - 2014-01-12 14:03:11.829583 Z
- sva <[email protected]>
- assigned to release v0.10 from v0.9
- ""
- - 2014-02-11 22:37:38.045628 Z
- sva <[email protected]>
- assigned to release v0.11 from v0.10
- ""
- - 2014-02-18 13:22:36.557661 Z
- sva <[email protected]>
- unassigned from release v0.11
- ""