groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Unchecked mallocs #79

Closed rayray closed 10 years ago

rayray commented 10 years ago

Submission of our app to Veracode has revealed a couple unchecked mallocs, one perhaps mitigated.

GRMustacheKeyAccess.m line 260

void *buffer = malloc(methodReturnLength);
[invocation getReturnValue:buffer];

There is no defined action in the documentation for getReturnValue: if buffer happens to be NULL.

Perhaps less important, GRMustacheContext.m line 330

GRMustacheContext **unsafeContexts = malloc(count * sizeof(GRMustacheContext *));
CFDictionaryGetKeysAndValues(unsafeContextForContext, NULL, (const void **)unsafeContexts);

In this case CFDictionaryGetKeysAndValues accepts NULL for the last argument, but I'm not sure if this is acceptable.

I can create a PR to handle these issues if you anyone can offer guidance on how potential NULL returns from malloc should be handled.

groue commented 10 years ago

Hi @rmndk,

Thanks for running such code analysis. In both cases, a malloc() failure can not be recovered, and the application should terminate immediately.

I don't quite know how to handle this kind of error properly: if malloc fails, then the memory is low, isn't it? Can we throw a regular exception, or should we brutally exit(1)? Do you have any advice?

groue commented 10 years ago

Second thought: library users may well be quite unhappy if third party code has them application crash. However I don't even know if I could allocate any NSError object after a failure of those mallocs: the requested size can safely be considered quite low in general.

groue commented 10 years ago

And I don't know any usual API that provides an NSError parameter for the sole purpose of memory error notification. It looks like we have nothing left than some flavor of exit(1), and exception raising. Actually I don't even know which kind of exception we should raise since the good-smelling NSMallocException is obsolete)...

OK - I'm waiting for your answer now.

groue commented 10 years ago

Any idea, @rmndk?

rayray commented 10 years ago

Sorry, work has been busy the last few days.

I don't have much guidance to offer, since I'm only referencing StackOverflow/Google at this point. Consensus seems to be to kill the app, but I'm sure you'd anger a lot of users that way. :)

These methods could just return nil, but then NSError would probably be useful. That would mean a fair amount of refactoring to send it back up to the caller.

groue commented 10 years ago

All right, this is fixed. It will ship in the next release.

groue commented 10 years ago

Shipped in v7.1.0