Skip to content

P
protobuf
Commit d6590d65 authored by Thomas Van Lenten's avatar Thomas Van Lenten
Browse files
Options

Drop all use of OSSpinLock 

Apple engineers have pointed out that OSSpinLocks are vulnerable to live locking
on iOS in cases of priority inversion:
. http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
. https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html

- Use a dispatch_semaphore_t within the extension registry.
- Use a dispatch_semaphore_t for protecting autocreation within messages.
- Drop the custom/internal GPBString class since we don't have really good
  numbers to judge the locking replacements and it isn't required. We can
  always bring it back with real data in the future.
parent afbc89a2
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 42 additions and 43 deletions
Makefile.am
View file @ d6590d65
......@@ -468,7 +468,6 @@ objectivec_EXTRA_DIST= \
objectivec/Tests/GPBMessageTests.m \
objectivec/Tests/GPBObjectiveCPlusPlusTest.mm \
objectivec/Tests/GPBPerfTests.m \
objectivec/Tests/GPBStringTests.m \
objectivec/Tests/GPBSwiftTests.swift \
objectivec/Tests/GPBTestUtilities.h \
objectivec/Tests/GPBTestUtilities.m \
......
objectivec/GPBCodedInputStream.m
View file @ d6590d65
This diff is collapsed.
objectivec/GPBCodedInputStream_PackagePrivate.h
View file @ d6590d65
......@@ -39,19 +39,6 @@
@class GPBUnknownFieldSet;
@class GPBFieldDescriptor;
// GPBString is a string subclass that avoids the overhead of initializing
// a full NSString until it is actually needed. Lots of protocol buffers contain
// strings, and instantiating all of those strings and having them parsed to
// verify correctness when the message was being read was expensive, when many
// of the strings were never being used.
//
// Note for future-self. I tried implementing this using a NSProxy.
// Turned out the performance was horrible in client apps because folks
// like to use libraries like SBJSON that grab characters one at a time.
// The proxy overhead was a killer.
@interface GPBString : NSString
@end
typedef struct GPBCodedInputStreamState {
const uint8_t *bytes;
size_t bufferSize;
......@@ -92,10 +79,6 @@ typedef struct GPBCodedInputStreamState {
CF_EXTERN_C_BEGIN
// Returns a GPBString with a +1 retain count.
GPBString *GPBCreateGPBStringWithUTF8(const void *bytes, NSUInteger length)
__attribute__((ns_returns_retained));
int32_t GPBCodedInputStreamReadTag(GPBCodedInputStreamState *state);
double GPBCodedInputStreamReadDouble(GPBCodedInputStreamState *state);
......
objectivec/GPBMessage.m
View file @ d6590d65
......@@ -568,13 +568,13 @@ static id GetArrayIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
id array = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
if (!array) {
// Check again after getting the lock.
OSSpinLockLock(&self->readOnlyMutex_);
dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
array = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
if (!array) {
array = CreateArrayForField(field, self);
GPBSetAutocreatedRetainedObjectIvarWithField(self, field, array);
}
OSSpinLockUnlock(&self->readOnlyMutex_);
dispatch_semaphore_signal(self->readOnlySemaphore_);
}
return array;
}
......@@ -598,13 +598,13 @@ static id GetMapIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
id dict = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
if (!dict) {
// Check again after getting the lock.
OSSpinLockLock(&self->readOnlyMutex_);
dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
dict = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
if (!dict) {
dict = CreateMapForField(field, self);
GPBSetAutocreatedRetainedObjectIvarWithField(self, field, dict);
}
OSSpinLockUnlock(&self->readOnlyMutex_);
dispatch_semaphore_signal(self->readOnlySemaphore_);
}
return dict;
}
......@@ -810,7 +810,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
messageStorage_ = (GPBMessage_StoragePtr)(
((uint8_t *)self) + class_getInstanceSize([self class]));
readOnlyMutex_ = OS_SPINLOCK_INIT;
readOnlySemaphore_ = dispatch_semaphore_create(1);
}
return self;
......@@ -1723,7 +1723,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
}
// Check for an autocreated value.
OSSpinLockLock(&readOnlyMutex_);
dispatch_semaphore_wait(readOnlySemaphore_, DISPATCH_TIME_FOREVER);
value = [autocreatedExtensionMap_ objectForKey:extension];
if (!value) {
// Auto create the message extensions to match normal fields.
......@@ -1740,7 +1740,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
[value release];
}
OSSpinLockUnlock(&readOnlyMutex_);
dispatch_semaphore_signal(readOnlySemaphore_);
return value;
}
......
objectivec/GPBMessage_PackagePrivate.h
View file @ d6590d65
......@@ -62,7 +62,12 @@ typedef struct GPBMessage_Storage *GPBMessage_StoragePtr;
// by *read* operations such as getters (autocreation of message fields and
// message extensions, not setting of values). Used to guarantee thread safety
// for concurrent reads on the message.
OSSpinLock readOnlyMutex_;
// NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
// pointed out that they are vulnerable to live locking on iOS in cases of
// priority inversion:
// http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
// https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
dispatch_semaphore_t readOnlySemaphore_;
}
// Gets an extension value without autocreating the result if not found. (i.e.
......
objectivec/GPBRootObject.m
View file @ d6590d65
......@@ -31,7 +31,6 @@
#import "GPBRootObject_PackagePrivate.h"
#import <objc/runtime.h>
#import <libkern/OSAtomic.h>
#import <CoreFoundation/CoreFoundation.h>
......@@ -96,13 +95,19 @@ static CFHashCode GPBRootExtensionKeyHash(const void *value) {
return jenkins_one_at_a_time_hash(key);
}
static OSSpinLock gExtensionSingletonDictionaryLock_ = OS_SPINLOCK_INIT;
// NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
// pointed out that they are vulnerable to live locking on iOS in cases of
// priority inversion:
// http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
// https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
static dispatch_semaphore_t gExtensionSingletonDictionarySemaphore;
static CFMutableDictionaryRef gExtensionSingletonDictionary = NULL;
static GPBExtensionRegistry *gDefaultExtensionRegistry = NULL;
+ (void)initialize {
// Ensure the global is started up.
if (!gExtensionSingletonDictionary) {
gExtensionSingletonDictionarySemaphore = dispatch_semaphore_create(1);
CFDictionaryKeyCallBacks keyCallBacks = {
// See description above for reason for using custom dictionary.
0,
......@@ -134,9 +139,10 @@ static GPBExtensionRegistry *gDefaultExtensionRegistry = NULL;
+ (void)globallyRegisterExtension:(GPBExtensionDescriptor *)field {
const char *key = [field singletonNameC];
OSSpinLockLock(&gExtensionSingletonDictionaryLock_);
dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore,
DISPATCH_TIME_FOREVER);
CFDictionarySetValue(gExtensionSingletonDictionary, key, field);
OSSpinLockUnlock(&gExtensionSingletonDictionaryLock_);
dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
}
static id ExtensionForName(id self, SEL _cmd) {
......@@ -166,14 +172,24 @@ static id ExtensionForName(id self, SEL _cmd) {
key[classNameLen] = '_';
memcpy(&key[classNameLen + 1], selName, selNameLen);
key[classNameLen + 1 + selNameLen] = '\0';
OSSpinLockLock(&gExtensionSingletonDictionaryLock_);
// NOTE: Even though this method is called from another C function,
// gExtensionSingletonDictionarySemaphore and gExtensionSingletonDictionary
// will always be initialized. This is because this call flow is just to
// lookup the Extension, meaning the code is calling an Extension class
// message on a Message or Root class. This guarantees that the class was
// initialized and Message classes ensure their Root was also initialized.
NSAssert(gExtensionSingletonDictionary, @"Startup order broken!");
dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore,
DISPATCH_TIME_FOREVER);
id extension = (id)CFDictionaryGetValue(gExtensionSingletonDictionary, key);
if (extension) {
// The method is getting wired in to the class, so no need to keep it in
// the dictionary.
CFDictionaryRemoveValue(gExtensionSingletonDictionary, key);
}
OSSpinLockUnlock(&gExtensionSingletonDictionaryLock_);
dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
return extension;
}
......
objectivec/GPBUtilities.m
View file @ d6590d65
......@@ -411,7 +411,7 @@ id GPBGetObjectIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
return field.defaultValue.valueMessage;
}
OSSpinLockLock(&self->readOnlyMutex_);
dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
GPBMessage *result = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
if (!result) {
// For non repeated messages, create the object, set it and return it.
......@@ -420,7 +420,7 @@ id GPBGetObjectIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
result = GPBCreateMessageWithAutocreator(field.msgClass, self, field);
GPBSetAutocreatedRetainedObjectIvarWithField(self, field, result);
}
OSSpinLockUnlock(&self->readOnlyMutex_);
dispatch_semaphore_signal(self->readOnlySemaphore_);
return result;
}
......
objectivec/ProtocolBuffers_OSX.xcodeproj/project.pbxproj
View file @ d6590d65
......@@ -28,7 +28,6 @@
8B8B615D17DF7056002EE618 /* GPBARCUnittestProtos.m in Sources */ = {isa = PBXBuildFile; fileRef = 8B8B615C17DF7056002EE618 /* GPBARCUnittestProtos.m */; settings = {COMPILER_FLAGS = "-fobjc-arc"; }; };
8B96157414C8C38C00A2AC0B /* GPBDescriptor.m in Sources */ = {isa = PBXBuildFile; fileRef = 8B96157314C8C38C00A2AC0B /* GPBDescriptor.m */; };
8B96157514CA019D00A2AC0B /* Descriptor.pbobjc.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BD3982214BE5B0C0081D629 /* Descriptor.pbobjc.m */; };
8BA9364518DA5F4C0056FA2A /* GPBStringTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */; };
8BBEA4A9147C727D00C4ADB7 /* GPBCodedInputStreamTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B69B0F94FDF800A0C422 /* GPBCodedInputStreamTests.m */; };
8BBEA4AA147C727D00C4ADB7 /* GPBCodedOuputStreamTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B69D0F94FDF800A0C422 /* GPBCodedOuputStreamTests.m */; };
8BBEA4AC147C727D00C4ADB7 /* GPBMessageTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B6A30F94FDF800A0C422 /* GPBMessageTests.m */; };
......@@ -158,7 +157,6 @@
8B8B615C17DF7056002EE618 /* GPBARCUnittestProtos.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = GPBARCUnittestProtos.m; sourceTree = "<group>"; };
8B96157214C8B06000A2AC0B /* GPBDescriptor.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = GPBDescriptor.h; sourceTree = "<group>"; };
8B96157314C8C38C00A2AC0B /* GPBDescriptor.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = GPBDescriptor.m; sourceTree = "<group>"; };
8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = GPBStringTests.m; sourceTree = "<group>"; };
8BBD9DB016DD1DC8008E1EC1 /* unittest_lite.proto */ = {isa = PBXFileReference; lastKnownFileType = text; name = unittest_lite.proto; path = ../../src/google/protobuf/unittest_lite.proto; sourceTree = "<group>"; };
8BBEA4A6147C727100C4ADB7 /* UnitTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = UnitTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
8BCF338814ED799900BC5317 /* GPBProtocolBuffers.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = GPBProtocolBuffers.m; sourceTree = "<group>"; };
......@@ -419,7 +417,6 @@
F4487C7E1AAF62CD00531423 /* GPBMessageTests+Serialization.m */,
F4B51B1D1BBC610700744318 /* GPBObjectiveCPlusPlusTest.mm */,
F41C175C1833D3310064ED4D /* GPBPerfTests.m */,
8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */,
8B4248BA1A8C256A00BC1EC6 /* GPBSwiftTests.swift */,
7461B6AB0F94FDF800A0C422 /* GPBTestUtilities.h */,
7461B6AC0F94FDF800A0C422 /* GPBTestUtilities.m */,
......@@ -689,7 +686,6 @@
F41C175D1833D3310064ED4D /* GPBPerfTests.m in Sources */,
F4353D341AC06F10005A6198 /* GPBDictionaryTests+Bool.m in Sources */,
F4487C831AAF6AB300531423 /* GPBMessageTests+Merge.m in Sources */,
8BA9364518DA5F4C0056FA2A /* GPBStringTests.m in Sources */,
8BBEA4B6147C727D00C4ADB7 /* GPBUnknownFieldSetTest.m in Sources */,
F4353D371AC06F10005A6198 /* GPBDictionaryTests+String.m in Sources */,
F4353D381AC06F10005A6198 /* GPBDictionaryTests+UInt32.m in Sources */,
......
objectivec/ProtocolBuffers_iOS.xcodeproj/project.pbxproj
View file @ d6590d65
......@@ -36,7 +36,6 @@
8B9A5EB41831993600A9D33B /* AppDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 8B9A5EB31831993600A9D33B /* AppDelegate.m */; };
8B9A5EB61831993600A9D33B /* Images.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 8B9A5EB51831993600A9D33B /* Images.xcassets */; };
8B9A5EEC18330A0F00A9D33B /* UIKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 8B9A5E9F1831913D00A9D33B /* UIKit.framework */; };
8BA9364518DA5F4C0056FA2A /* GPBStringTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */; };
8BBEA4A9147C727D00C4ADB7 /* GPBCodedInputStreamTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B69B0F94FDF800A0C422 /* GPBCodedInputStreamTests.m */; };
8BBEA4AA147C727D00C4ADB7 /* GPBCodedOuputStreamTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B69D0F94FDF800A0C422 /* GPBCodedOuputStreamTests.m */; };
8BBEA4AC147C727D00C4ADB7 /* GPBMessageTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 7461B6A30F94FDF800A0C422 /* GPBMessageTests.m */; };
......@@ -179,7 +178,6 @@
8B9A5EAD1831993600A9D33B /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = en; path = en.lproj/InfoPlist.strings; sourceTree = "<group>"; };
8B9A5EB31831993600A9D33B /* AppDelegate.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AppDelegate.m; sourceTree = "<group>"; };
8B9A5EB51831993600A9D33B /* Images.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Images.xcassets; sourceTree = "<group>"; };
8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = GPBStringTests.m; sourceTree = "<group>"; };
8BBD9DB016DD1DC8008E1EC1 /* unittest_lite.proto */ = {isa = PBXFileReference; lastKnownFileType = text; name = unittest_lite.proto; path = ../../src/google/protobuf/unittest_lite.proto; sourceTree = "<group>"; };
8BBEA4A6147C727100C4ADB7 /* UnitTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = UnitTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
8BCF338814ED799900BC5317 /* GPBProtocolBuffers.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = GPBProtocolBuffers.m; sourceTree = "<group>"; };
......@@ -457,7 +455,6 @@
F4487C801AAF62FC00531423 /* GPBMessageTests+Serialization.m */,
F4B51B1B1BBC5C7100744318 /* GPBObjectiveCPlusPlusTest.mm */,
F41C175C1833D3310064ED4D /* GPBPerfTests.m */,
8BA9364418DA5F4B0056FA2A /* GPBStringTests.m */,
8B4248B31A8BD96E00BC1EC6 /* GPBSwiftTests.swift */,
7461B6AB0F94FDF800A0C422 /* GPBTestUtilities.h */,
7461B6AC0F94FDF800A0C422 /* GPBTestUtilities.m */,
......@@ -785,7 +782,6 @@
F41C175D1833D3310064ED4D /* GPBPerfTests.m in Sources */,
F4353D421AC06F31005A6198 /* GPBDictionaryTests+Bool.m in Sources */,
F4487C851AAF6AC500531423 /* GPBMessageTests+Merge.m in Sources */,
8BA9364518DA5F4C0056FA2A /* GPBStringTests.m in Sources */,
8BBEA4B6147C727D00C4ADB7 /* GPBUnknownFieldSetTest.m in Sources */,
F4353D451AC06F31005A6198 /* GPBDictionaryTests+String.m in Sources */,
F4353D461AC06F31005A6198 /* GPBDictionaryTests+UInt32.m in Sources */,
......
objectivec/Tests/GPBCodedInputStreamTests.m
View file @ d6590d65
......@@ -266,6 +266,9 @@
}
// Verifies fix for b/10315336.
// Note: Now that there isn't a custom string class under the hood, this test
// isn't as critical, but it does cover bad input and if a custom class is added
// again, it will help validate that class' handing of bad utf8.
- (void)testReadMalformedString {
NSOutputStream* rawOutput = [NSOutputStream outputStreamToMemory];
GPBCodedOutputStream* output =
......@@ -276,7 +279,7 @@
[output writeRawVarint32:tag];
[output writeRawVarint32:5];
// Create an invalid utf-8 byte array.
uint8_t bytes[5] = {0xc2, 0xf2};
uint8_t bytes[] = {0xc2, 0xf2, 0x0, 0x0, 0x0};
[output writeRawData:[NSData dataWithBytes:bytes length:sizeof(bytes)]];
[output flush];
......@@ -286,6 +289,7 @@
TestAllTypes* message = [TestAllTypes parseFromCodedInputStream:input
extensionRegistry:nil
error:NULL];
XCTAssertNotNil(message);
// Make sure we can read string properties twice without crashing.
XCTAssertEqual([message.defaultString length], (NSUInteger)0);
XCTAssertEqualObjects(@"", message.defaultString);
......
objectivec/Tests/GPBStringTests.m deleted 100644 → 0
View file @ afbc89a2
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment