Skip to content

Commit aada9c4

Browse files
Benjamin PoulainBenjaminPoulain
Benjamin Poulain
authored and
BenjaminPoulain
committed
Fix unsafe memory load/store from the argument encoder/decoder affecting ARM
https://bugs.webkit.org/show_bug.cgi?id=125674 Patch by Benjamin Poulain <bpoulain@apple.com> on 2013-12-12 Reviewed by Darin Adler. Depending on the CPU and CPU config, load and store may have to be aligned. The argument buffer has no particular alignment which can cause problems. In this case, on ARMv7, strd and ldrd can have alignment restriction on 16 bytes. The code encoding double and 64 bits integers was causing bugs. To avoid problems, the encoders/decoders are modified to just use memcpy. The compiler optimizes it away for the right instructions (clang uses two ldr/str in the case of 64bits values on ARMv7). * Platform/CoreIPC/ArgumentDecoder.cpp: (CoreIPC::decodeValueFromBuffer): (CoreIPC::ArgumentDecoder::decode): * Platform/CoreIPC/ArgumentEncoder.cpp: (CoreIPC::copyValueToBuffer): (CoreIPC::ArgumentEncoder::encode): Canonical link: https://commits.webkit.org/143699@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@160529 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent b2728b4 commit aada9c4

3 files changed

Lines changed: 57 additions & 39 deletions

File tree

‎Source/WebKit2/ChangeLog‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,26 @@
1+
2013-12-12 Benjamin Poulain <bpoulain@apple.com>
2+
3+
Fix unsafe memory load/store from the argument encoder/decoder affecting ARM
4+
https://bugs.webkit.org/show_bug.cgi?id=125674
5+
6+
Reviewed by Darin Adler.
7+
8+
Depending on the CPU and CPU config, load and store may have to be aligned.
9+
The argument buffer has no particular alignment which can cause problems.
10+
11+
In this case, on ARMv7, strd and ldrd can have alignment restriction on 16 bytes.
12+
The code encoding double and 64 bits integers was causing bugs.
13+
14+
To avoid problems, the encoders/decoders are modified to just use memcpy. The compiler optimizes
15+
it away for the right instructions (clang uses two ldr/str in the case of 64bits values on ARMv7).
16+
17+
* Platform/CoreIPC/ArgumentDecoder.cpp:
18+
(CoreIPC::decodeValueFromBuffer):
19+
(CoreIPC::ArgumentDecoder::decode):
20+
* Platform/CoreIPC/ArgumentEncoder.cpp:
21+
(CoreIPC::copyValueToBuffer):
22+
(CoreIPC::ArgumentEncoder::encode):
23+
124
2013-12-12 Tim Horton <timothy_horton@apple.com>
225

326
Build fix for 32-bit.

‎Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -126,13 +126,19 @@ bool ArgumentDecoder::decodeVariableLengthByteArray(DataReference& dataReference
126126
return true;
127127
}
128128

129+
template<typename Type>
130+
static void decodeValueFromBuffer(Type& value, uint8_t*& bufferPosition)
131+
{
132+
memcpy(&value, bufferPosition, sizeof(value));
133+
bufferPosition += sizeof(Type);
134+
}
135+
129136
bool ArgumentDecoder::decode(bool& result)
130137
{
131138
if (!alignBufferPosition(sizeof(result), sizeof(result)))
132139
return false;
133140

134-
result = *reinterpret_cast<bool*>(m_bufferPos);
135-
m_bufferPos += sizeof(result);
141+
decodeValueFromBuffer(result, m_bufferPos);
136142
return true;
137143
}
138144

@@ -141,8 +147,7 @@ bool ArgumentDecoder::decode(uint8_t& result)
141147
if (!alignBufferPosition(sizeof(result), sizeof(result)))
142148
return false;
143149

144-
result = *reinterpret_cast<uint8_t*>(m_bufferPos);
145-
m_bufferPos += sizeof(result);
150+
decodeValueFromBuffer(result, m_bufferPos);
146151
return true;
147152
}
148153

@@ -151,18 +156,16 @@ bool ArgumentDecoder::decode(uint16_t& result)
151156
if (!alignBufferPosition(sizeof(result), sizeof(result)))
152157
return false;
153158

154-
result = *reinterpret_cast_ptr<uint16_t*>(m_bufferPos);
155-
m_bufferPos += sizeof(result);
159+
decodeValueFromBuffer(result, m_bufferPos);
156160
return true;
157161
}
158162

159163
bool ArgumentDecoder::decode(uint32_t& result)
160164
{
161165
if (!alignBufferPosition(sizeof(result), sizeof(result)))
162166
return false;
163-
164-
result = *reinterpret_cast_ptr<uint32_t*>(m_bufferPos);
165-
m_bufferPos += sizeof(result);
167+
168+
decodeValueFromBuffer(result, m_bufferPos);
166169
return true;
167170
}
168171

@@ -171,8 +174,7 @@ bool ArgumentDecoder::decode(uint64_t& result)
171174
if (!alignBufferPosition(sizeof(result), sizeof(result)))
172175
return false;
173176

174-
result = *reinterpret_cast_ptr<uint64_t*>(m_bufferPos);
175-
m_bufferPos += sizeof(result);
177+
decodeValueFromBuffer(result, m_bufferPos);
176178
return true;
177179
}
178180

@@ -181,28 +183,25 @@ bool ArgumentDecoder::decode(int32_t& result)
181183
if (!alignBufferPosition(sizeof(result), sizeof(result)))
182184
return false;
183185

184-
result = *reinterpret_cast_ptr<uint32_t*>(m_bufferPos);
185-
m_bufferPos += sizeof(result);
186+
decodeValueFromBuffer(result, m_bufferPos);
186187
return true;
187188
}
188189

189190
bool ArgumentDecoder::decode(int64_t& result)
190191
{
191192
if (!alignBufferPosition(sizeof(result), sizeof(result)))
192193
return false;
193-
194-
result = *reinterpret_cast_ptr<uint64_t*>(m_bufferPos);
195-
m_bufferPos += sizeof(result);
194+
195+
decodeValueFromBuffer(result, m_bufferPos);
196196
return true;
197197
}
198198

199199
bool ArgumentDecoder::decode(float& result)
200200
{
201201
if (!alignBufferPosition(sizeof(result), sizeof(result)))
202202
return false;
203-
204-
result = *reinterpret_cast_ptr<float*>(m_bufferPos);
205-
m_bufferPos += sizeof(result);
203+
204+
decodeValueFromBuffer(result, m_bufferPos);
206205
return true;
207206
}
208207

@@ -211,8 +210,7 @@ bool ArgumentDecoder::decode(double& result)
211210
if (!alignBufferPosition(sizeof(result), sizeof(result)))
212211
return false;
213212

214-
result = *reinterpret_cast_ptr<double*>(m_bufferPos);
215-
m_bufferPos += sizeof(result);
213+
decodeValueFromBuffer(result, m_bufferPos);
216214
return true;
217215
}
218216

‎Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.cpp‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -123,67 +123,64 @@ void ArgumentEncoder::encodeVariableLengthByteArray(const DataReference& dataRef
123123
encodeFixedLengthData(dataReference.data(), dataReference.size(), 1);
124124
}
125125

126+
template<typename Type>
127+
static void copyValueToBuffer(Type value, uint8_t* bufferPosition)
128+
{
129+
memcpy(bufferPosition, &value, sizeof(Type));
130+
}
131+
126132
void ArgumentEncoder::encode(bool n)
127133
{
128134
uint8_t* buffer = grow(sizeof(n), sizeof(n));
129-
130-
*reinterpret_cast<bool*>(buffer) = n;
135+
copyValueToBuffer(n, buffer);
131136
}
132137

133138
void ArgumentEncoder::encode(uint8_t n)
134139
{
135140
uint8_t* buffer = grow(sizeof(n), sizeof(n));
136-
137-
*reinterpret_cast<uint8_t*>(buffer) = n;
141+
copyValueToBuffer(n, buffer);
138142
}
139143

140144
void ArgumentEncoder::encode(uint16_t n)
141145
{
142146
uint8_t* buffer = grow(sizeof(n), sizeof(n));
143-
144-
*reinterpret_cast_ptr<uint16_t*>(buffer) = n;
147+
copyValueToBuffer(n, buffer);
145148
}
146149

147150
void ArgumentEncoder::encode(uint32_t n)
148151
{
149152
uint8_t* buffer = grow(sizeof(n), sizeof(n));
150-
151-
*reinterpret_cast_ptr<uint32_t*>(buffer) = n;
153+
copyValueToBuffer(n, buffer);
152154
}
153155

154156
void ArgumentEncoder::encode(uint64_t n)
155157
{
156158
uint8_t* buffer = grow(sizeof(n), sizeof(n));
157-
158-
*reinterpret_cast_ptr<uint64_t*>(buffer) = n;
159+
copyValueToBuffer(n, buffer);
159160
}
160161

161162
void ArgumentEncoder::encode(int32_t n)
162163
{
163164
uint8_t* buffer = grow(sizeof(n), sizeof(n));
164-
165-
*reinterpret_cast_ptr<int32_t*>(buffer) = n;
165+
copyValueToBuffer(n, buffer);
166166
}
167167

168168
void ArgumentEncoder::encode(int64_t n)
169169
{
170170
uint8_t* buffer = grow(sizeof(n), sizeof(n));
171-
172-
*reinterpret_cast_ptr<int64_t*>(buffer) = n;
171+
copyValueToBuffer(n, buffer);
173172
}
174173

175174
void ArgumentEncoder::encode(float n)
176175
{
177176
uint8_t* buffer = grow(sizeof(n), sizeof(n));
178-
179-
*reinterpret_cast_ptr<float*>(buffer) = n;
177+
copyValueToBuffer(n, buffer);
180178
}
181179

182180
void ArgumentEncoder::encode(double n)
183181
{
184182
uint8_t* buffer = grow(sizeof(n), sizeof(n));
185-
186-
*reinterpret_cast_ptr<double*>(buffer) = n;
183+
copyValueToBuffer(n, buffer);
187184
}
188185

189186
void ArgumentEncoder::addAttachment(const Attachment& attachment)

0 commit comments

Comments
 (0)