Use objcType to identify the value types. #73
Conversation
| static BOOL IsValueType(id someValue, char *objCType) { | ||
| if ([someValue isKindOfClass:[NSValue class]]) { | ||
| NSValue *asValue = (NSValue *)someValue; | ||
| return strcmp(asValue.objCType, objCType) == 0; |
There was a problem hiding this comment.
Please switch to strncmp and limit the length to something short like 10 characters.
CGSize = "{CGSize=dd}"
CGRect = "{CGRect={CGPoint=dd}{CGSize=dd}}"
CGPoint = "{CGPoint=dd}"
CGRectNull = "{CGRect={CGPoint=dd}{CGSize=dd}}"
There was a problem hiding this comment.
Fixed by using objCType's string length. objCType is "trusted" in that we're only ever providing @encode values to it.
| static BOOL IsValueType(id someValue, char *objCType) { | ||
| if ([someValue isKindOfClass:[NSValue class]]) { | ||
| NSValue *asValue = (NSValue *)someValue; | ||
| return strncmp(asValue.objCType, objCType, strlen(objCType)) == 0; |
There was a problem hiding this comment.
If you're going to trust one of the strings, I would trust the asValue.objCType and not the char * passed in.
This whole thing seems safer as a list of functions:
static BOOL IsCGSizeValueType(id someValue);
static BOOL IsCGPointValueType(id someValue);
static BOOL IsCGRectValueType(id someValue);There was a problem hiding this comment.
My thinking is that id someValue comes from user-land, while objCType comes from this file, so the vector of attack for objCType is smaller than that of id someValue.
Can you help me understand your thinking?
There was a problem hiding this comment.
Ah, I can see how you feel the specifically-typed methods would help make this safer, but I'm not certain that there's a large benefit when the APIs are private and scoped to this file. What do you think?
There was a problem hiding this comment.
I was thinking more of the "let's make this more general" risk later on. Someone copy-pastes this out into a header or utility class and keeps the behavior. In that case, the classes passed to this function are more likely to be first-party, but the char * could be anything.
Since within this class we only ever pass the result of the compiler @encode it's safe. I don't think the really is a good "generic" as well as buffer-safe approach. I'm OK merging this as-is since it is currently a static, internal function .
There was a problem hiding this comment.
Agreed with that thinking. I'll tidy this up so that copy-pasting is safer.
This makes it much more scalable to support arbitrary key paths.
This makes it much more scalable to support arbitrary key paths.