Changeset 206151 in webkit

Timestamp:

Sep 20, 2016 10:14:00 AM (8 years ago)

Author:

Jonathan Bedard

Message:

Undefined behavior: Left shift negative number
​https://bugs.webkit.org/show_bug.cgi?id=161866

Reviewed by Keith Miller.

Left shifting a negative number is undefined behavior in C/C++, although most implementations do define it. Explicitly clarifying the intended behavior due to shifting negative number in some cases.

Source/JavaScriptCore:

• dfg/DFGAbstractHeap.h:

(JSC::DFG::AbstractHeap::encode): Explicitly cast signed integer for left shift.

Source/WTF:

• wtf/text/Base64.cpp:

(WTF::base64EncodeInternal): Changed signed character to unsigned when shifting.
(WTF::base64Encode): Ditto.
(WTF::base64URLEncode): Ditto.
(WTF::base64DecodeInternal): Ditto.

• wtf/text/Base64.h: Ditto.

(WTF::SignedOrUnsignedCharVectorAdapter): Rebuilt to stop using union as a bitwise_cast.
(WTF::ConstSignedOrUnsignedCharVectorAdapter): Ditto.

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/dfg/DFGAbstractHeap.h (modified) (1 diff)
• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/text/Base64.cpp (modified) (5 diffs)
• WTF/wtf/text/Base64.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-09-20  Jonathan Bedard  <jbedard@apple.com>
+
+        Undefined behavior: Left shift negative number
+        https://bugs.webkit.org/show_bug.cgi?id=161866
+
+        Reviewed by Keith Miller.
+
+        Left shifting a negative number is undefined behavior in C/C++, although most implementations do define it. Explicitly clarifying the intended behavior due to shifting negative number in some cases.
+
+        * dfg/DFGAbstractHeap.h:
+        (JSC::DFG::AbstractHeap::encode): Explicitly cast signed integer for left shift.
+
 2016-09-20  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGAbstractHeap.h

@@ -299 +299 @@
         int64_t kindAsInt = static_cast<int64_t>(kind);
         ASSERT(kindAsInt < (1 << topShift));
-        return kindAsInt | (payload.isTop() << topShift) | (payload.valueImpl() << valueShift);
+        return kindAsInt | (static_cast<uint64_t>(payload.isTop()) << topShift) | (bitwise_cast<uint64_t>(payload.valueImpl()) << valueShift);
     }
     

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-09-20  Jonathan Bedard  <jbedard@apple.com>
+
+        Undefined behavior: Left shift negative number
+        https://bugs.webkit.org/show_bug.cgi?id=161866
+
+        Reviewed by Keith Miller.
+
+        Left shifting a negative number is undefined behavior in C/C++, although most implementations do define it. Explicitly clarifying the intended behavior due to shifting negative number in some cases.
+
+        * wtf/text/Base64.cpp:
+        (WTF::base64EncodeInternal): Changed signed character to unsigned when shifting.
+        (WTF::base64Encode): Ditto.
+        (WTF::base64URLEncode): Ditto.
+        (WTF::base64DecodeInternal): Ditto.
+        * wtf/text/Base64.h: Ditto.
+        (WTF::SignedOrUnsignedCharVectorAdapter): Rebuilt to stop using union as a bitwise_cast.
+        (WTF::ConstSignedOrUnsignedCharVectorAdapter): Ditto.
+
 2016-09-19  Daniel Bates  <dabates@apple.com>
 

trunk/Source/WTF/wtf/text/Base64.cpp

@@ -93 +93 @@
 };
 
-static inline void base64EncodeInternal(const char* data, unsigned len, Vector<char>& out, Base64EncodePolicy policy, const char (&encodeMap)[64])
+static inline void base64EncodeInternal(const unsigned char* data, unsigned len, Vector<char>& out, Base64EncodePolicy policy, const char (&encodeMap)[64])
 {
     out.clear();
@@ -161 +161 @@
 {
     Vector<char> result;
-    base64EncodeInternal(static_cast<const char*>(data), length, result, policy, base64EncMap);
+    base64EncodeInternal(static_cast<const unsigned char*>(data), length, result, policy, base64EncMap);
     return String(result.data(), result.size());
 }
@@ -167 +167 @@
 void base64Encode(const void* data, unsigned len, Vector<char>& out, Base64EncodePolicy policy)
 {
-    base64EncodeInternal(static_cast<const char*>(data), len, out, policy, base64EncMap);
+    base64EncodeInternal(static_cast<const unsigned char*>(data), len, out, policy, base64EncMap);
 }
 
@@ -173 +173 @@
 {
     Vector<char> result;
-    base64EncodeInternal(static_cast<const char*>(data), length, result, Base64URLPolicy, base64URLEncMap);
+    base64EncodeInternal(static_cast<const unsigned char*>(data), length, result, Base64URLPolicy, base64URLEncMap);
     return String(result.data(), result.size());
 }
@@ -179 +179 @@
 void base64URLEncode(const void* data, unsigned len, Vector<char>& out)
 {
-    base64EncodeInternal(static_cast<const char*>(data), len, out, Base64URLPolicy, base64URLEncMap);
+    base64EncodeInternal(static_cast<const unsigned char*>(data), len, out, Base64URLPolicy, base64URLEncMap);
 }
 
 template<typename T>
-static inline bool base64DecodeInternal(const T* data, unsigned length, Vector<char>& out, unsigned options, const char (&decodeMap)[128])
+static inline bool base64DecodeInternal(const T* data, unsigned length, SignedOrUnsignedCharVectorAdapter& out, unsigned options, const char (&decodeMap)[128])
 {
     out.clear();

trunk/Source/WTF/wtf/text/Base64.h

@@ -49 +49 @@
 class SignedOrUnsignedCharVectorAdapter {
 public:
-    SignedOrUnsignedCharVectorAdapter(Vector<char>& vector) { m_vector.c = &vector; }
-    SignedOrUnsignedCharVectorAdapter(Vector<uint8_t>& vector) { m_vector.u = &vector; }
-
-    operator Vector<char>&() { return *m_vector.c; }
-    void clear() { m_vector.c->clear(); }
+    SignedOrUnsignedCharVectorAdapter(Vector<char>& vector)
+        : m_isSigned(true)
+    {
+        m_vector.c = &vector;
+    }
+    SignedOrUnsignedCharVectorAdapter(Vector<uint8_t>& vector)
+        : m_isSigned(false)
+    {
+        m_vector.u = &vector;
+    }
+
+    uint8_t* data()
+    {
+        if (m_isSigned)
+            return reinterpret_cast<uint8_t*>(m_vector.c->data());
+        return m_vector.u->data();
+    }
+    
+    size_t size() const
+    {
+        if (m_isSigned)
+            return m_vector.c->size();
+        return m_vector.u->size();
+    }
+    
+    void clear()
+    {
+        if (m_isSigned) {
+            m_vector.c->clear();
+            return;
+        }
+        m_vector.u->clear();
+    }
+    
+    void grow(size_t size)
+    {
+        if (m_isSigned) {
+            m_vector.c->grow(size);
+            return;
+        }
+        m_vector.u->grow(size);
+    }
+    
+    void shrink(size_t size)
+    {
+        if (m_isSigned) {
+            m_vector.c->shrink(size);
+            return;
+        }
+        m_vector.u->shrink(size);
+    }
+    
+    uint8_t& operator[](size_t position) { return data()[position]; }
 
 private:
+    bool m_isSigned;
     union {
         Vector<char>* c;
@@ -64 +113 @@
 class ConstSignedOrUnsignedCharVectorAdapter {
 public:
-    ConstSignedOrUnsignedCharVectorAdapter(const Vector<char>& vector) { m_vector.c = &vector; }
-    ConstSignedOrUnsignedCharVectorAdapter(const Vector<uint8_t>& vector) { m_vector.u = &vector; }
-
-    operator const Vector<char>&() { return *m_vector.c; }
-    const char* data() const { return m_vector.c->data(); }
-    size_t size() const { return m_vector.c->size(); }
+    ConstSignedOrUnsignedCharVectorAdapter(const Vector<char>& vector)
+        : m_isSigned(false)
+    {
+        m_vector.c = &vector;
+    }
+    ConstSignedOrUnsignedCharVectorAdapter(const Vector<uint8_t>& vector)
+        : m_isSigned(true)
+    {
+        m_vector.u = &vector;
+    }
+
+    const uint8_t* data() const
+    {
+        if (m_isSigned)
+            return reinterpret_cast<const uint8_t*>(m_vector.c->data());
+        return m_vector.u->data();
+    }
+    size_t size() const
+    {
+        if (m_isSigned)
+            return m_vector.c->size();
+        return m_vector.u->size();
+    }
 
 private:
+    bool m_isSigned;
     union {
         const Vector<char>* c;