Replace -[NSKeyedUnarchiver unarchiveObjectWithData:] so it doesn't crash on corrupt archive
-
If -[NSKeyedUnarchiver unarchiveObjectWithData:] is handed a corrupt
archive, it raises an exception AND crashes the program. I find this
to be very annoying since, almost always, archives come from files or
network sources where corruption is possible. Therefore, I have
always protected this invocation in a try/catch block.
As I was about to do this for about the fifth time this year, I
decided to use the Method Replacement feature of Leopard to replace
this method, once and for all, with one that wouldn't crash. I
understand that Method Replacement should not be done casually because
any plug-in code and even Cocoa itself will use the replaced method.
However, I can't think of any usage where a crash would be required
behavior.
It seems to work fine, after 10 minutes of testing. Does anyone see
any problem with this?
Sincerely,
Jerry Krinock
#import <Cocoa/Cocoa.h>
/*!
@brief Improvements to NSKeyedUnarchiver
@details Method +unarchiveObjectWithData: has been replaced so that
instead of raising an exception and crashing if given a corrupt
archive,
it just returns nil. Also, another method has been added which
returns the exception.
*/
@interface NSKeyedUnarchiver (CatchExceptions)
/*!
@brief Like unarchiveObjectWithData:, except it returns the
exception by reference.
@param exception_p Pointer which will, upon return, if an
exception occurred and said pointer is not NULL, point to said
NSException.
*/
+ (id)unarchiveObjectWithData:(NSData*)data
exception_p:(NSException**)exception_p ;
@end
#import "NSKeyedUnarchiver+CatchExceptions.h"
#import <objc/runtime.h>
@implementation NSKeyedUnarchiver (CatchExceptions)
+ (id)unarchiveObjectWithData:(NSData*)data
exception_p:(NSException**)exception_p {
id object = nil ;
@try {
// Note: Since methods were swapped, this is invoking the
original method
object = [NSKeyedUnarchiver
replacement_unarchiveObjectWithData:data] ;
}
@catch (NSException* exception) {
if (exception_p) {
*exception_p = exception ;
}
}
@finally{
}
return object ;
}
+ (void)load {
// Swap the implementations of +unarchiveObjectWithData: and
+replacement_unarchiveObjectWithData:.
// When the +unarchiveObjectWithData: message is sent to the
NSKeyedUnarchiver class object,
// +replacement_unarchiveObjectWithData: will be invoked
instead. Conversely,
// +replacement_unarchiveObjectWithData: will invoke
+unarchiveObjectWithData:.
Method originalMethod = class_getClassMethod(self,
@selector(unarchiveObjectWithData:)) ;
Method replacedMethod = class_getClassMethod(self,
@selector(replacement_unarchiveObjectWithData:)) ;
method_exchangeImplementations(originalMethod, replacedMethod) ;
}
+ (id)replacement_unarchiveObjectWithData:(NSData*)data {
return [self unarchiveObjectWithData:data
exception_p:NULL] ;
}
@end
TEST CODE:
int main (int argc, const char * argv[]) {
NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
// Try to unarchive your bash profile
// This ain't gonna be able to unarchive
NSData* data = [NSData dataWithContentsOfFile:[NSHomeDirectory()
stringByAppendingPathComponent:@".bash_profile"]] ;
NSLog(@"Your bash profile data is %d bytes.", [data length]) ;
id whatever ;
// Try it using the normal method
whatever = [NSKeyedUnarchiver unarchiveObjectWithData:data] ;
NSLog(@"1. unarchived whatever = %@", whatever) ;
// Try it using the improved method that returns the exception:
NSException** exception ;
whatever = [NSKeyedUnarchiver unarchiveObjectWithData:data
exception_p:&exception] ;
NSLog(@"2. unarchived whatever = %@", whatever) ;
NSLog(@"exception = %@", exception) ;
[pool release] ;
} -
On Thu, Jul 2, 2009 at 11:17 PM, Jerry Krinock<jerry...> wrote:> If -[NSKeyedUnarchiver unarchiveObjectWithData:] is handed a corrupt
> archive, it raises an exception AND crashes the program. ÃÂ I find this to be
> very annoying since, almost always, archives come from files or network
> sources where corruption is possible. ÃÂ Therefore, I have always protected
> this invocation in a try/catch block.
>
> As I was about to do this for about the fifth time this year, I decided to
> use the Method Replacement feature of Leopard to replace this method, once
> and for all, with one that wouldn't crash. ÃÂ I understand that Method
> Replacement should not be done casually because any plug-in code and even
> Cocoa itself will use the replaced method. ÃÂ However, I can't think of any
> usage where ÃÂ a crash would be required behavior.
>
> It seems to work fine, after 10 minutes of testing. ÃÂ Does anyone see any
> problem with this?
Is there a reason you can't just implement a new method in a category
and not replace the existing one? It will be just as safe in your
code, just as easy to use, and it won't run the potential risk of
screwing up code that you don't control.
Mike -
On 2009 Jul 02, at 21:11, Michael Ash wrote:> Is there a reason you can't just implement a new method in a category
> and not replace the existing one? It will be just as safe in your
> code, just as easy to use,
I'd considered doing that and yes it could be done. But then it would
have a different method name that I'd have to remember to use instead
of -unarchiveObjectWithData:.
Replacing a Cocoa method seems like it should only be done if the
original method shipped by Apple is buggy. I believe that's the case
here. The only behavior I'm losing is a potential crash.
I suppose Method Replacement has disadvantages too. I have to
remember to include this method replacement file in any new project I
start, or else I'll get the old crashy behavior, since I'll be out of
the habit of using try/catch.> and it won't run the potential risk of screwing up code that you
> don't control.
Interesting -- but the only "screwing up" I'm doing is allowing it to
execute beyond a point where it otherwise would have crashed. I
suppose that if someone is using this crashing "feature" of -
unarchiveObjectWithData: as some kind of self-destruct security
mechanism, I could be introducing a security hole :))
Anyhow, I get your point.
Thanks, Michael. -
On Thu, Jul 2, 2009 at 11:17 PM, Jerry Krinock <jerry...> wrote:
If -[NSKeyedUnarchiver unarchiveObjectWithData:] is handed a corrupt> archive, it raises an exception AND crashes the program.
Do you have a sample app & data combo which demonstrates the crash?
+[NSKeyedUnarchiver unarchiveObjectWithData:] is documented to raise an
exception when feed an incomprehensible archive.
If you have no exception handler in place, the program will terminate. But
that isn't due to a bug in NSKeyedUnarchiver - it is just working as
advertised - whether or not you like the advertised behavior.
- Jim -
On 2009 Jul 02, at 22:53, Jim Correia wrote:> Do you have a sample app & data combo which demonstrates the crash?
Yes, if you take the TEST CODE at the bottom of my original post and
revert to Apple's method by commenting out the +[NSKeyedUnarchiver
load] method in there, it will crash.> If you have no exception handler in place, the program will
> terminate. But that isn't due to a bug in NSKeyedUnarchiver - it is
> just working as advertised - whether or not you like the advertised
> behavior.
Indeed, if I enclose it in a @try block, then it will not crash. But
I think it's error-prone and inconvenient to have to enclose it in a
@try block.
Is that what you mean by "exception handler in place"? -- enclose it
in a @try block ? -
On Fri, Jul 3, 2009 at 1:42 AM, Jerry Krinock<jerry...> wrote:>
> On 2009 Jul 02, at 21:11, Michael Ash wrote:
>
>> Is there a reason you can't just implement a new method in a category
>> and not replace the existing one? It will be just as safe in your
>> code, just as easy to use,
>
> I'd considered doing that and yes it could be done. ÃÂ But then it would have
> a different method name that I'd have to remember to use instead of
> -unarchiveObjectWithData:.
>
> Replacing a Cocoa method seems like it should only be done if the original
> method shipped by Apple is buggy. ÃÂ I believe that's the case here. ÃÂ The only
> behavior I'm losing is a potential crash.
Well, that's wrong. The behavior you're losing is that of throwing an
exception on error. It is entirely possible that Apple's code relies
on this method throwing an exception on error. As your own code
demonstrates, you can catch the exception in question without
crashing.
You're not fixing a bug, unless you consider throwing an exception to
always be a bug AND you consider any code which relies on catching an
exception to likewise be a bug. You're changing existing non-buggy
behavior. That's a bad thing to do unless you have a VERY good reason,
and IMHO "a different method name that I'd have to remember to use" is
a pretty crappy reason for this.
Mike -
On 2009 Jul 03, at 05:50, Michael Ash wrote:> It is entirely possible that Apple's code relies
> on this method throwing an exception on error.
Ah, you're correct. So I've changed it to be a regular category
method, +unarchiveObjectSafelyWithData:, instead of replacing the
method.
