From 33da4715ea4ffc723223eb223baa726e15462e5a Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Fri, 20 Sep 2019 10:52:34 -0400 Subject: [PATCH 1/2] [ObjC] Don't use unions and instead use memcpy for the type swaps. (#6672) The code in question hasn't change in a long time so the cause of https://github.com/firebase/firebase-ios-sdk/issues/3851 still appears to be an Xcode 11 clang change/bug; but this does appear to be slightly better code for the work being done. Cleanup along the way for #6679 --- objectivec/GPBUtilities_PackagePrivate.h | 28 ++++++++++++++---------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/objectivec/GPBUtilities_PackagePrivate.h b/objectivec/GPBUtilities_PackagePrivate.h index ed424ce399..336a745ca1 100644 --- a/objectivec/GPBUtilities_PackagePrivate.h +++ b/objectivec/GPBUtilities_PackagePrivate.h @@ -71,27 +71,31 @@ GPB_INLINE void GPBDebugCheckRuntimeVersion() { // Conversion functions for de/serializing floating point types. GPB_INLINE int64_t GPBConvertDoubleToInt64(double v) { - union { double f; int64_t i; } u; - u.f = v; - return u.i; + GPBInternalCompileAssert(sizeof(double) == sizeof(int64_t), double_not_64_bits); + int64_t result; + memcpy(&result, &v, sizeof(result)); + return result; } GPB_INLINE int32_t GPBConvertFloatToInt32(float v) { - union { float f; int32_t i; } u; - u.f = v; - return u.i; + GPBInternalCompileAssert(sizeof(float) == sizeof(int32_t), float_not_32_bits); + int32_t result; + memcpy(&result, &v, sizeof(result)); + return result; } GPB_INLINE double GPBConvertInt64ToDouble(int64_t v) { - union { double f; int64_t i; } u; - u.i = v; - return u.f; + GPBInternalCompileAssert(sizeof(double) == sizeof(int64_t), double_not_64_bits); + double result; + memcpy(&result, &v, sizeof(result)); + return result; } GPB_INLINE float GPBConvertInt32ToFloat(int32_t v) { - union { float f; int32_t i; } u; - u.i = v; - return u.f; + GPBInternalCompileAssert(sizeof(float) == sizeof(int32_t), float_not_32_bits); + float result; + memcpy(&result, &v, sizeof(result)); + return result; } GPB_INLINE int32_t GPBLogicalRightShift32(int32_t value, int32_t spaces) { From 397e017c8024cef060cfcdf870f264424c189791 Mon Sep 17 00:00:00 2001 From: dmaclach Date: Fri, 20 Sep 2019 08:19:45 -0700 Subject: [PATCH 2/2] Remove OSReadLittle* due to alignment requirements (#6678) The OSReadLittleInt64 function as defined by Apple reduces down to: `return *(volatile uint64_t *)((uintptr_t)base + byteOffset);` which means we are type-punning using a cast. On ARMv7 and other aligned architectures this can cause crashes. Minimal example: https://gist.github.com/dmaclach/b10b0a71ae614d304c067cb9bd264336 Fixes #6679 --- objectivec/GPBCodedInputStream.m | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/objectivec/GPBCodedInputStream.m b/objectivec/GPBCodedInputStream.m index 57d04dde38..4c8d640fda 100644 --- a/objectivec/GPBCodedInputStream.m +++ b/objectivec/GPBCodedInputStream.m @@ -93,14 +93,22 @@ static int8_t ReadRawByte(GPBCodedInputStreamState *state) { static int32_t ReadRawLittleEndian32(GPBCodedInputStreamState *state) { CheckSize(state, sizeof(int32_t)); - int32_t value = OSReadLittleInt32(state->bytes, state->bufferPos); + // Not using OSReadLittleInt32 because it has undocumented dependency + // on reads being aligned. + int32_t value; + memcpy(&value, state->bytes + state->bufferPos, sizeof(int32_t)); + value = OSSwapLittleToHostInt32(value); state->bufferPos += sizeof(int32_t); return value; } static int64_t ReadRawLittleEndian64(GPBCodedInputStreamState *state) { CheckSize(state, sizeof(int64_t)); - int64_t value = OSReadLittleInt64(state->bytes, state->bufferPos); + // Not using OSReadLittleInt64 because it has undocumented dependency + // on reads being aligned. + int64_t value; + memcpy(&value, state->bytes + state->bufferPos, sizeof(int64_t)); + value = OSSwapLittleToHostInt64(value); state->bufferPos += sizeof(int64_t); return value; }