From 1635c02e920013906dfb1c8fa62d9ec3393f9312 Mon Sep 17 00:00:00 2001 From: Ashok Menon Date: Mon, 20 Feb 2017 04:28:32 -0800 Subject: [PATCH] Simplifying Struct definition. Summary: Since we are reading from a file, we should make sure this struct is packed, just in case we change it down the line and the compiler decides it might want to introduce padding, we're now protected against that. There was also a discussion about the fact that people might use `ptr += sizeof(BundleHeader)` as an idiom in their code, which would currently be incorrect, if padding was introduced at the end of the file. Actually, it remains incorrect to do that now, because a RAM bundle header is a different size to a BC Bundle header. If people are properly testing their code, they should spot this pretty quickly, because it will always be an incorrect thing to do with a RAM bundle, so this isn't as bad as previously thought: where the code only succeeds when the compiler deigns to not pad the struct at the end. This diff also cleans up how headers are initialised. `BundleHeader` has a constructor that explicitly zero-initialises it so we can rely on the default initializer to do the right thing now. Reviewed By: mhorowitz Differential Revision: D4572032 fbshipit-source-id: 7dc50cfa9438dfdfb9f842dc39d8f15334813c63 --- React/Base/RCTJavaScriptLoader.mm | 6 +++--- React/CxxBridge/RCTCxxBridge.mm | 2 +- React/Executors/RCTJSCExecutor.mm | 2 +- .../main/jni/xreact/jni/CatalystInstanceImpl.cpp | 2 +- ReactCommon/cxxreact/JSBundleType.cpp | 14 +++++++------- ReactCommon/cxxreact/JSBundleType.h | 10 ++++------ ReactCommon/cxxreact/JSCExecutor.cpp | 2 +- 7 files changed, 18 insertions(+), 20 deletions(-) diff --git a/React/Base/RCTJavaScriptLoader.mm b/React/Base/RCTJavaScriptLoader.mm index 60fa2c682..9258cea73 100755 --- a/React/Base/RCTJavaScriptLoader.mm +++ b/React/Base/RCTJavaScriptLoader.mm @@ -114,7 +114,7 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init) return nil; } - facebook::react::BundleHeader header{}; + facebook::react::BundleHeader header; size_t readResult = fread(&header, sizeof(header), 1, bundle); fclose(bundle); if (readResult != 1) { @@ -151,11 +151,11 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init) } return nil; } - else if ((uint32_t)runtimeBCVersion != header.BCVersion) { + else if ((uint32_t)runtimeBCVersion != header.version) { if (error) { NSString *errDesc = [NSString stringWithFormat:@"BC Version Mismatch. Expect: %d, Actual: %u", - runtimeBCVersion, header.BCVersion]; + runtimeBCVersion, header.version]; *error = [NSError errorWithDomain:RCTJavaScriptLoaderErrorDomain code:RCTJavaScriptLoaderErrorBCVersion diff --git a/React/CxxBridge/RCTCxxBridge.mm b/React/CxxBridge/RCTCxxBridge.mm index c8d19c14e..8964d1618 100644 --- a/React/CxxBridge/RCTCxxBridge.mm +++ b/React/CxxBridge/RCTCxxBridge.mm @@ -87,7 +87,7 @@ static JSValueRef nativePerformanceNow( } static bool isRAMBundle(NSData *script) { - BundleHeader header{}; + BundleHeader header; [script getBytes:&header length:sizeof(header)]; return parseTypeFromHeader(header) == ScriptTag::RAMBundle; } diff --git a/React/Executors/RCTJSCExecutor.mm b/React/Executors/RCTJSCExecutor.mm index 92918feb6..3b5a1816f 100644 --- a/React/Executors/RCTJSCExecutor.mm +++ b/React/Executors/RCTJSCExecutor.mm @@ -706,7 +706,7 @@ static TaggedScript loadTaggedScript(NSData *script, { RCT_PROFILE_BEGIN_EVENT(0, @"executeApplicationScript / prepare bundle", nil); - facebook::react::BundleHeader header{}; + facebook::react::BundleHeader header; [script getBytes:&header length:sizeof(header)]; facebook::react::ScriptTag tag = facebook::react::parseTypeFromHeader(header); diff --git a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp index 77cd311d3..b4aa91b63 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/CatalystInstanceImpl.cpp @@ -194,7 +194,7 @@ bool CatalystInstanceImpl::isIndexedRAMBundle(const char *sourcePath) { if (!bundle_stream) { return false; } - BundleHeader header{}; + BundleHeader header; bundle_stream.read(reinterpret_cast(&header), sizeof(header)); bundle_stream.close(); return parseTypeFromHeader(header) == ScriptTag::RAMBundle; diff --git a/ReactCommon/cxxreact/JSBundleType.cpp b/ReactCommon/cxxreact/JSBundleType.cpp index a53e00977..071abd7f3 100644 --- a/ReactCommon/cxxreact/JSBundleType.cpp +++ b/ReactCommon/cxxreact/JSBundleType.cpp @@ -7,18 +7,18 @@ namespace facebook { namespace react { static uint32_t constexpr RAMBundleMagicNumber = 0xFB0BD1E5; -static uint64_t constexpr BCBundleMagicNumber = 0xFF4865726D657300; +static uint32_t constexpr BCBundleMagicNumber = 0x6D657300; ScriptTag parseTypeFromHeader(const BundleHeader& header) { - if (littleEndianToHost(header.RAMMagic) == RAMBundleMagicNumber) { + + switch (littleEndianToHost(header.magic)) { + case RAMBundleMagicNumber: return ScriptTag::RAMBundle; - } - - if (littleEndianToHost(header.BCMagic) == BCBundleMagicNumber) { + case BCBundleMagicNumber: return ScriptTag::BCBundle; + default: + return ScriptTag::String; } - - return ScriptTag::String; } const char *stringForScriptTag(const ScriptTag& tag) { diff --git a/ReactCommon/cxxreact/JSBundleType.h b/ReactCommon/cxxreact/JSBundleType.h index 1945fa4c7..3b89ecb2b 100644 --- a/ReactCommon/cxxreact/JSBundleType.h +++ b/ReactCommon/cxxreact/JSBundleType.h @@ -27,16 +27,14 @@ enum struct ScriptTag { * 4 bytes, for BC bundles this is 12 bytes. This structure holds the first 12 * bytes from a bundle in a way that gives access to that information. */ -union BundleHeader { +struct __attribute__((packed)) BundleHeader { BundleHeader() { std::memset(this, 0, sizeof(BundleHeader)); } - uint32_t RAMMagic; - struct { - uint64_t BCMagic; - uint32_t BCVersion; - }; + uint32_t magic; + uint32_t reserved_; + uint32_t version; }; /** diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp index 4dd5139b3..bfc35e76f 100644 --- a/ReactCommon/cxxreact/JSCExecutor.cpp +++ b/ReactCommon/cxxreact/JSCExecutor.cpp @@ -386,7 +386,7 @@ void JSCExecutor::loadApplicationScript(std::unique_ptr scrip } } #elif defined(__APPLE__) - BundleHeader header{}; + BundleHeader header; memcpy(&header, script->c_str(), std::min(script->size(), sizeof(BundleHeader))); auto scriptTag = parseTypeFromHeader(header);