1. Jeff Laing
  2. MobileDeviceAccess
Issue #2 resolved

Crash in -[AFCDirectoryAccess copyRemoteFile:toLocalFile:]

Volodymyr Sapsai
created an issue

Steps to reproduce:

  1. Try to copy a file larger than 10K with -[AFCDirectoryAccess copyRemoteFile:toLocalFile:].

Actual result:

Crash sometimes when we try to [in readN:bufsz bytes:buff]; (line 996) the second time.

Expected result:

No crash.

Discussion:

I cannot reproduce the issue myself, it was discovered by my colleague. He has discovered the following. When we call [in readN:bufsz bytes:buff], it calls AFCFileRefRead which corrupts the stack. The first call succeeded, but the second call fails due to corrupted stack. And AFCFileRefRead corrupts the stack because it has incorrect signature. Looks like len should be uint64_t * instead of uint32_t *.

Comments (15)

  1. Jeff Laing repo owner

    I can't say I have seen it, but at a guess this will be the result of Apple upgrading the mobile framework.

    What version of iTunes do you/he have installed?

  2. Volodymyr Sapsai reporter

    I think it was iTunes 11.0.2 or 11.0.3. Now we are using iTunes 11.1.1, but we've thrown away corresponding code. So I don't know for sure if iTunes 11.1.1 has the same problem with AFCFileRefRead.

  3. Jeff Laing repo owner

    Hmmm, I know what you are saying, but I am not experiencing it. I just quite successfully copied a 1.26GB archive back using copyRemoteFile:toLocalFile: and it worked fine.

    Aha, the MobileDevice.Framework is fat - I'll bet that all my test programs are compiled 32-bit and yours is 64-bit. You might try recompiling for 32-bit to see if that helps. Not that that is a solution, but it is something to try.

  4. Jeff Laing repo owner

    Damn, I can see things don't work when I build it in 64-bit mode. That will definitely be the problem. That shouldn't take too long to work through.

  5. Jeff Laing repo owner

    Are you sure you have the latest version? There was a checkin a few versions ago that changed that routine from using NSMutableData to using malloc() because there seemed to be an issue with something moving the NSMutableData block around. Someone else did report a problem in that space.

    To reiterate, I am happy to update the code if we can identify what is wrong, but at this stage, I'm still not sure...

  6. Volodymyr Sapsai reporter

    I am unable to reproduce the crash in -[AFCDirectoryAccess copyRemoteFile:toLocalFile:], but with the following code in -[AFCFileReference readN:bytes:]

    if (![self ensureFileIsOpen]) return NO;
    const uint32_t kPaddingValue = 0xcccccccc;
    uint32_t paddingBuffer[] = {kPaddingValue, kPaddingValue, kPaddingValue, kPaddingValue};
    uint32_t *afcSizePtr = paddingBuffer + 1;
    *afcSizePtr = n;
    afc_error_t status = AFCFileRefRead(_afc, _ref, buff, afcSizePtr);
    NSAssert(paddingBuffer[2] == kPaddingValue, nil);
    if (![self checkStatus:status from:"AFCFileRefRead"]) return 0;
    return *afcSizePtr;
    

    NSAssert fails because paddingBuffer[2] is overwritten with 0. I.e. AFCFileRefRead writes to afcSizePtr as to uint64_t*.

    It's for 64-bit binary. Jeff, can you please try to reproduce the described behavior with provided snippet?

  7. Nathan Van Fleet

    Is there a work around to get this working? I get this every time I try to download a file from the phone. It successfully transfers the whole file and then crashes as described earlier.

  8. Volodymyr Sapsai reporter

    As far as I remember, we've fixed the crash by changing

    afc_error_t AFCFileRefRead(afc_connection conn,afc_file_ref ref,void *buf,uint32_t *len);

    to

    afc_error_t AFCFileRefRead(afc_connection conn,afc_file_ref ref,void *buf,uint64_t *len);

    Though take this recommendation with grain of salt, because I haven't tested it thoroughly and it is unclear in which iTunes release uint32_t* was changed to uint64_t*.

  9. Jeff Laing repo owner

    Ok, I finally got back to this, and from looking with Hopper, I can definitely see that the prototypes for a number of functions are wrong, and need to be quad words. I believe I have nailed them all, and should be able to upload the latest version sometime soon.

    The upshot, however, will be that readN:bytes: has to change prototype slightly - instead of returning/accepting "uint32_t" lengths, it is going to accept something like "afc_long" - that type will conditionally be 32/64 bit depending on how you build the library.

  10. Jeff Laing repo owner

    I believe I have fixed this with the latest commit. As mentioned above, I have changed a couple of prototypes to take the 32/64-bit sense into account.

    I have also cleaned up a bunch of other stuff, so you should probably test it a little carefully.

  11. Log in to comment