Possible bug in RMStoreTransactionReceiptVerificator

[John-Lluch]

Hi,

Just looking at your code I found this pattern on several places

NSError *error;
NSData *data = [NSURLConnection sendSynchronousRequest:request returningResponse:nil error:&error];
dispatch_async(dispatch_get_main_queue(), ^{
if (error != nil)
{
// do stuff

I read somewhere that errors returned by reference are not guaranteed to be initialized unless the condition returned by the method is false. If this is the case you should check for data being nil instead of the error being not nil. If the error was indeed not set by the method then it may contain garbage on success. I suppose you actually tested this and it worked for this particular method call but you can not assume that it will always work as per the general rule.

Thanks,

[hpique]

Most likely you mean this quote from the Error Handling Programming Guide:

Important: Success or failure is indicated by the return value of the method. Although Cocoa methods that indirectly return error objects in the Cocoa error domain are guaranteed to return such objects if the method indicates failure by directly returning nil or NO, you should always check that the return value is nil or NO before attempting to do anything with the NSError object.

To date, none of the methods with indirect errors used by RMStore return garbage, and it would require a very oblivious developer at Apple to change this. However, it would be safer to use the return value to check for success.

Want to submit a pull request fixing the ones you find?

That said, I find the documentation of the return value of sendSynchronousRequest:returningResponse:error: ambiguous.

The downloaded data for the URL request. Returns nil if a connection could not be created or if the download fails.

This is not the same than "Returns nil if any error is encountered." How can we be sure that the method will not return partial data under any circumstance?

[John-Lluch]

Hi Hermes,

Yes that excerpt is exactly the one that I remember having read. Other occurrences of this pattern on the RMStore classes are found around calls to JSONObjectWithData:options:error:, in this case the docs are clear enough:

error. If an error occurs, upon return contains an NSError object that describes the problem,
returns a Foundation object from the JSON data in data, or nil if an error occurs.

So for this case it is clear that we should check the returned value instead of the error object.

Now, the documentation on the sendSynchronousRequest:returningResponse:error: is not that explicit but it still states what it does, read through the end.

According to my previous experience, methods returning an NSError by reference do not set the passed in value unless there is an actual error. This is why I am surprised -and confused- about why the RMStore does not crash. I have not tested it, only looked at the code, but I clearly see that the verifyRequestData method may return failure when there is no error, and thus the app would crash when attempting to dereference the underlying error error object. This is at least what would have happened before ARC. My only guess is that maybe ARC initializes everything to nil even it it is declared on the local scope, so in fact NSError *error; is equivalent to NSError *error=nil; This is the only reason for why it would not crash. But I don't think this is the case.

In any case, I think that it would be safer to test for nil against the returned NSData. On all my previous uses of the method sendSynchronousRequest:returningResponse:error I found that:

1- If data is nil it is always because an error and the error is set.
2 -The method can return some data on error, but on this case the error is not set. This is consistent with what we should expect. The error in this case can be detected by checking the statusCode property of the returningResponse.

So after all what the docs state is exactly what happen:

The downloaded data for the URL request. Returns nil if a connection could not be created or if the download fails.

Yes, exactly. And the error is set in such case. But a connection could be made with no error and still be a failure, this is then checked through the statusCode on the returned NSHTTPURLResponse

Joan