Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZSTD on iOS - alignment to 32 bits required? #1210

Closed
vade opened this issue Jun 26, 2018 · 3 comments
Closed

ZSTD on iOS - alignment to 32 bits required? #1210

vade opened this issue Jun 26, 2018 · 3 comments
Labels

Comments

@vade
Copy link

vade commented Jun 26, 2018

Hi

Firstly, thanks for ZSTD!

I've been chasing down a crasher in my iOS app which leverages ZSTD 1.3.4, and I occasionally get a crash in MEM_write32 when compressing with a context with no dictionary:

    NSData* metadata = (snip)
    const size_t expectedCompressionSize = ZSTD_compressBound(metadata.length);
    
    UInt8* compressionBuffer = malloc(expectedCompressionSize);

    size_t const compressedSize = ZSTD_compressCCtx(compressionContext, compressionBuffer, expectedCompressionSize, metadata.bytes, (size_t) metadata.length, 1);
    
    // Hit error on compression use ZSTD_getErrorName for error reporting eventually.
    if(ZSTD_isError(compressedSize))
    {
        free(compressionBuffer);
        return nil;
    }
    
    NSData* zstdCompressedData = [[NSData alloc] initWithBytesNoCopy:compressionBuffer length:compressedSize freeWhenDone:YES];

Is there any alignment requirements or nuances for deployment on iOS / Arm? I've noticed a few align=32 in make files but didnt see anything specific in the docs for iOS. Are there specific clang alignments?

Are there any nuances with using a ZSTD_compressCCtx in a serial dispatch queue (not a single thread, but a guaranteed serialized invocation of a context instance that may be used across a few system threads but never be concurrent)?

Thanks for any information!

@Cyan4973
Copy link
Contributor

MEM_write32() is supposed to work with any target, including those requiring strict alignments.
There can be some subtleties with some uncommon low power devices, but for iOS ones, I don't expect any problem. Besides, most modern iOS devices use an ARM 64-bit derivative, which is able to read / write on unaligned addresses, like a desktop x86 cpu.
So I would be very surprised if alignment was an issue in this case.

Another possibility is that MEM_write32() writes at an unauthorized address.
Why it would happen is not clear.
ZSTD_compressCCtx() is supposed to be safe vs buffer overflows.

You also mention working in a multi-threaded environment.
A given context must only be used by one thread at a time.
It can switch thread. But the really important thing is that no 2 threads should use it at the same time.
I understand your implementation is supposed to take care of this condition.
But I would invite to look again.
Sometimes, it can be pretty hard to guarantee such condition, depending on implementation details.

I would recommend a run of your application using a thread sanitizer. It should be able to flag any mis-usage, if there is any.

@vade
Copy link
Author

vade commented Jun 26, 2018

Thanks @Cyan4973 for the clarifications for alignment and threading WRT to access of a ZSTD compression context. I believe I follow the rule you mention (its safe for strictly serial access across various threads, but absolutely not safe for concurrent access). I also know how hard it can truly be to guarantee that is the case :) dispatch_async by way of Apples libDispatch/Grand Central Dispatch should allow for strict adherence but perhaps I am not as slick as I like to think I am :)

Feel free to close this issue as its a question rather than a bug, and you've clarified. If I find any specific crash behavior I can isolate or that prompts an additional question, I'll submit a new 'bug'.

Appreciate the thorough reply. Thank you!

@vade
Copy link
Author

vade commented Jun 26, 2018

You're hunch is totally dead on and threading is tricky. Thanks for the kick in the ass to triple check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants