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

crc32 function outputs wrong result for large data on the macOS arm64 platform #105967

Closed
maikschulze opened this issue Jun 21, 2023 · 18 comments · Fixed by #112615
Closed

crc32 function outputs wrong result for large data on the macOS arm64 platform #105967

maikschulze opened this issue Jun 21, 2023 · 18 comments · Fixed by #112615
Assignees
Labels
OS-mac stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@maikschulze
Copy link

maikschulze commented Jun 21, 2023

Bug report

The functions zlib.crc32 and binascii.crc32 share the problematic behavior. When computing the CRC for data >= 2GB macOS arm64 binaries result in different values than all other platforms such as macOS x64, Windows x64, Linux x64. Consequently, problems arise e.g. when using the zipfile module.

A clear and concise description of what the bug is.

Reproduction:

import random
random.seed(0)

import zlib
import binascii

def chunks(list, n):
  for i in range(0, len(list), n):
      yield list[i:i + n]

random_megabyte = random.randbytes(1024*1024)
random_1_gigabyte = random_megabyte * 1024 * 1
random_4_gigabyte = random_megabyte * 1024 * 4

crc_1_gigabyte_zlib = zlib.crc32(random_1_gigabyte, 0)
crc_1_gigabyte_binascii = binascii.crc32(random_1_gigabyte, 0)

crc_4_gigabyte_zlib = zlib.crc32(random_4_gigabyte, 0)
crc_4_gigabyte_binascii = binascii.crc32(random_4_gigabyte, 0)

# incremental computation in chunks < 2 GB fixes macOS arm64
chunked_crc_4_gigabyte_zlib = 0
chunked_crc_4_gigabyte_binascii = 0
for chunk in chunks(random_4_gigabyte, 1024 * 1024 * 1024 *1):
  chunked_crc_4_gigabyte_zlib = zlib.crc32(chunk, chunked_crc_4_gigabyte_zlib)
  chunked_crc_4_gigabyte_binascii = binascii.crc32(chunk, chunked_crc_4_gigabyte_binascii)

print("crc_1_gigabyte_zlib".ljust(32),             "expected: 0xe28bc234 computed:", hex(crc_1_gigabyte_zlib))
print("crc_1_gigabyte_binascii".ljust(32),         "expected: 0xe28bc234 computed:", hex(crc_1_gigabyte_binascii))
print("crc_4_gigabyte_zlib".ljust(32),             "expected: 0x278432d6 computed:", hex(crc_4_gigabyte_zlib))
print("crc_4_gigabyte_binascii".ljust(32),         "expected: 0x278432d6 computed:", hex(crc_4_gigabyte_binascii))
print("chunked_crc_4_gigabyte_zlib".ljust(32),     "expected: 0x278432d6 computed:", hex(chunked_crc_4_gigabyte_zlib))
print("chunked_crc_4_gigabyte_binascii".ljust(32), "expected: 0x278432d6 computed:", hex(chunked_crc_4_gigabyte_binascii))

Output on macOS arm64:

mac-arm64:crc_bug dev_admin$ /opt/homebrew/bin/python3 crc_bug_report.py 
crc_1_gigabyte_zlib              expected: 0xe28bc234 computed: 0xe28bc234
crc_1_gigabyte_binascii          expected: 0xe28bc234 computed: 0xe28bc234
crc_4_gigabyte_zlib              expected: 0x278432d6 computed: 0x6b54c6be
crc_4_gigabyte_binascii          expected: 0x278432d6 computed: 0x6b54c6be
chunked_crc_4_gigabyte_zlib      expected: 0x278432d6 computed: 0x278432d6
chunked_crc_4_gigabyte_binascii  expected: 0x278432d6 computed: 0x278432d6

Your environment

  • CPython versions tested on: Python 3.9.6 Python 3.11.4
  • Operating system and architecture: macOS arm64, macOS x64, Windows x64, Linux x64

Linked PRs

@maikschulze maikschulze added the type-bug An unexpected behavior, bug, or error label Jun 21, 2023
@terryjreedy terryjreedy added the stdlib Python modules in the Lib dir label Jun 21, 2023
@ned-deily ned-deily self-assigned this Jun 22, 2023
@ned-deily
Copy link
Member

Thanks for the report. The Python standard library zlib and binascii modules depend on the widely-used third-party zlib library (libz) to do much of their work. The misbehavior shown in your test case appears to be due to a bug of some sort in the native Apple Silicon (arm64) libz binary shipped by Apple with current releases of macOS. The incorrect results can be seen on Apple Silicon Macs running any current release of macOS that support arm64, that is, macOS 11 Big Sur, macOS 12 Monterey, and macOS 13 Ventura. The upcoming macOS 14 Sonoma, which is currently in restricted beta, appears likely to be released with an updated version of libz that does not have this problem.

The Apple-supplied libz on macOS 11 through 13 reports its version as 1.2.11. However, a quick look at the zlib project files published on Apple's open-source page (https://opensource.apple.com/releases/) shows that the Apple version has been significantly patched from the original upstream 1.2.11 release (linked to from https://www.zlib.net). I was unable to reproduce the failure here on either Apple Silicon or Intel Macs when using a libz built from unmodified upstream zlib source for 1.2.11, 1.2.12, or 1.2.13 (the most recent release) nor with current libz 1.2.13 binaries from either Homebrew or MacPorts. So, the problem seems to be confined to the Apple-supplied libz on Apple Silicon Macs.

By default, when building on macOS, cpython will dynamically link with a number of open-source libraries supplied with macOS, including libz. While there are advantages to this approach and has been encouraged by Apple in the past, it can be a disadvantage if the system-supplied version has a bug.

A non-exhaustive survey of several ways of using cpython on macOS:

  1. pythons provided by all current python.org macOS universal2 installers dynamically link to the Apple-supplied system libz and thus exhibit this problem. (A vary quick and rather dirty mitigation for python.org universal2 pythons is to force the use of Intel emulation on Apple Silicon Macs by installing Rosetta2 and using python3.11-intel64 instead of python3.11 to invoke the interpreter, all at the risk of degraded Python performance and other new bugs.)

  2. pythons provided by Homebrew also link to the system libz and exhibit this problem (spot checked with Homebrew's current Python 3.9 through 3.11).

  3. pythons provided by MacPorts dynamically link with a MacPorts-provided libz and thus do not exhibit this problem.

  4. By default, pythons built from source will link with the system libz and exhibit this problem. Linking with a non-system libz can be specified in the ./configure build step by various means including the setting of CFLAGS, CPPFLAGS, and LDFLAGS environment variables or, as of 3.11, with pkg-config if available.

This is not the first time that problems have been found using the zlib and binascii modules with crc32 and very large data buffers, for example, #54485 and, more recently, #82437. (The Python 3.9.6 supplied with current releases of Apple's developer tools exhibits both the problem identified in this issue and the problem in #82437 which was fixed in a later release of 3.9.)

So, assuming the above analysis is correct and that there is no problem within cpython itself, what should be done to mitigate this? Ideally, Apple would address the problem with an updated libz while all three affected macOS release families are still receiving updates. To that end, we should then ensure that an issue is open with Apple about this problem. Given the uncertainty of if and when such a fix would be available, distributors of cpython for macOS and users who build cpython from source will need to evaluate the risk to their users and applications. If necessary, builds can be changed to use a non-system libz. That would be fairly straight-forward for distributions like the python.org installers and Homebrew which already make use of third-party dynamic shared libraries. Stand-alone applications need to be careful when linking and shipping a new shared library, perhaps needing RPATH directives, or by forcing static linking with libz, something which macOS deliberately does not make easy to do.

Comments? Other options?

cc: @ronaldoussoren, @ambv, @gpshead. @Yhg1s

@ronaldoussoren
Copy link
Contributor

We should definitely file and issue with Apple about this, ideally with a reproducer in C. A fix in the macOS 14 beta doesn't bode well for getting this fixed in older releases though. That said, most fixes I've seen for issues I filed about system libraries like this were fixed in new major releases anyway.

Other than that I'm not sure. Do you know what kind of changes there are in the Apple for of libz and if those are relevant for our users (for example by enabling some kind of hardware offload)?

If the changes aren't relevant we could start shipping our own copy of libz, the build tooling is set up for that although it would add yet another moving part to track and be concerned about when there's a release that doesn't fit into the cpython release schedule.

Another option we could look into: Is it possible to tweak the our CRC32 bindings to call the libz function with small enough buffers to not trigger the issue? The disadvantage of that it that it would complicate our code base, but hopefully not too much.

@gpshead
Copy link
Member

gpshead commented Jun 22, 2023

@ned-deily libz is used by a huge pile of software other than Python, what do other things use instead of the system libz? While we could ship our own zlib on macOS but it'd be a shame as the standard zlib is very boring and unoptimized; dynamically linking against the platform one means they're responsible for security and performance updates.

@gpshead
Copy link
Member

gpshead commented Jun 22, 2023

ex: zip and unzip on macOS, what do those use? what about Java and Ruby?

@gpshead
Copy link
Member

gpshead commented Jun 22, 2023

From a risk perspective we already build our own upstream zlib 1.2.13 for Windows binary releases so statically linking against a build of that on macOS isn't a huge burden, zlib security fixes trigger Python binary security releases anyways.

#91349 (use zlib-chromium or zlib-ng) and #93819 (we need zlib in WASI) are tangentially related FWIW.

@gpshead
Copy link
Member

gpshead commented Jun 22, 2023

re: Apple bug vs our bug, we should verify that by writing a tiny C reproducer using libz to confirm the issue.

From a CPython bugfix perspective, we do still carry a copy of our own crc32 function within the binascii module, conditionally compiled in or out (based on zlib) - the condition in which that is used could be adjusted as a platform workaround. (but this still sounds like there may be other internal Apple libz issues with visible impact?)

@arhadthedev
Copy link
Member

we do still carry a copy of our own crc32 function within the binascii module, conditionally compiled in or out (based on zlib)

It will be eliminated by gh-32043 (see USE_ZLIB_CRC32 removals in Modules/binascii.c).

@markokr
Copy link

markokr commented Nov 13, 2023

If there is wish to continue to link with platform libz, how about setting the loop limit to 1G instead 4G-1 (UINT32_MAX)? That would isolate the code from platform corner-cases and its unlikely to give any measurable slowdown.

@ronaldoussoren
Copy link
Contributor

The following is a C program that tries to reproduce the issue based on the code in binascii.c:

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include "zlib.h"


int main(void)
{
    unsigned char* buffer = malloc(4L * 1024 * 1024 * 1024);
    if (buffer == NULL) {
        perror("malloc");
        return 2;
    }

    for (size_t k = 0; k < 4L * 1024 * 1024 * 1024; k++) {
        buffer[k] = k & 0xff;
    }

    unsigned int crc_whole = 0;
    unsigned int crc_chunked = 0;

    // use two chunks for crc_whole because 4G is larger than UINT_MAX,
    // 1 big chunk and 1 chunk for the rest
    crc_whole = crc32(crc_whole, buffer, 4L * 1024 * 1024 * 1024 - 1);
    crc_whole = crc32(crc_whole, buffer + 4L * 1024 * 1024 * 1024 - 1, 1);

    for (unsigned int i = 0; i < 4; i++) {
        crc_chunked = crc32(crc_chunked, buffer + (i * 1024 * 1024 * 1024), 1 * 1024 * 1024 * 1024);
    }

    printf("W: %#x\n", crc_whole);
    printf("C: %#x\n", crc_chunked);
    printf("W == C: %d\n", crc_whole == crc_chunked);

    return !(crc_whole == crc_chunked);
}

Build using cc -o repro repro.c -lz.

For me this prints:

% ./repro                                                                                                                                       (3.12)cpython-upstream
W: 0xe3bbf7be
C: 0xe3bbf7be
W == C: 1

That is, both approaches get the same CRC.

System version (M1 MacBook):

% sw_vers                                                                                                                                       (3.12)cpython-upstream
ProductName:		macOS
ProductVersion:		14.1.2
BuildVersion:		23B92

I get the same result in a VM running 10.15.5.

@ronaldoussoren
Copy link
Contributor

My results are expected given @ned-deily's earlier remarks...

Limiting chunk size in calls to crc32(3) to 1 GB is the easiest workaround on our end.

gpshead added a commit to gpshead/cpython that referenced this issue Dec 2, 2023
…GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
@gpshead
Copy link
Member

gpshead commented Dec 2, 2023

Can someone who can reproduce the original problem confirm if applying my #112615 change to limit the calls to 1gig solves it for them?

gpshead added a commit that referenced this issue Dec 4, 2023
…lls to 1gig (#112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 4, 2023
…API calls to 1gig (pythonGH-112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
(cherry picked from commit 4eddb4c)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 4, 2023
…API calls to 1gig (pythonGH-112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
(cherry picked from commit 4eddb4c)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this issue Dec 4, 2023
… API calls to 1gig (GH-112615) (#112725)

gh-105967: Work around a macOS bug, limit zlib C library crc32 API calls to 1gig (GH-112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
(cherry picked from commit 4eddb4c)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this issue Dec 4, 2023
… API calls to 1gig (GH-112615) (#112724)

gh-105967: Work around a macOS bug, limit zlib C library crc32 API calls to 1gig (GH-112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
(cherry picked from commit 4eddb4c)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@ned-deily
Copy link
Member

I've run some tests and your fix does seem to successfully workaround the Apple zlib bug on Apple Silicon Macs running macOS 12.6.1 Monterey and macOS 13.6.1 Ventura (I don't have access at the moment to macOS 11 Big Sur on an Apple Silicon Mac) and have checked that there is no regression on Intel Macs.

Interestingly, it appears that Apple has fixed the problem in at least the most recent release of macOS 14 Sonoma: on macOS 14.1.2, the most recent release of macOS, the test runs correctly on Apple Silicon without the fix (and with).

Thanks for the PR, @gpshead. What do you think about adding a test for this? It might show up again anywhere.

@gpshead
Copy link
Member

gpshead commented Dec 5, 2023

Thanks for the PR, @gpshead. What do you think about adding a test for this? It might show up again anywhere.

I was pondering that. I think what we're missing is bigmem test coverage for these being run on the relevant macOS platforms? I haven't gone looking to see if existing tests would've explicitly caught this though (if not, we should add some, and get one relevant buildbot set with a high enough memory limit on its regrtest command line regardless)

@gpshead gpshead reopened this Dec 5, 2023
@ronaldoussoren
Copy link
Contributor

Thanks for the PR, @gpshead. What do you think about adding a test for this? It might show up again anywhere.

I was pondering that. I think what we're missing is bigmem test coverage for these being run on the relevant macOS platforms? I haven't gone looking to see if existing tests would've explicitly caught this though (if not, we should add some, and get one relevant buildbot set with a high enough memory limit on its regrtest command line regardless)

Both binascii and zlib have bigmem tests that should have caught this:

class ChecksumBigBufferTestCase(unittest.TestCase):
"""bpo-38256 - check that inputs >=4 GiB are handled correctly."""
@bigmemtest(size=_4G + 4, memuse=1, dry_run=False)
def test_big_buffer(self, size):
data = b"nyan" * (_1G + 1)
self.assertEqual(binascii.crc32(data), 1044521549)

and

class ChecksumBigBufferTestCase(unittest.TestCase):
@bigmemtest(size=_4G + 4, memuse=1, dry_run=False)
def test_big_buffer(self, size):
data = b"nyan" * (_1G + 1)
self.assertEqual(zlib.crc32(data), 1044521549)

Buildbots run tests with python -m test --slow-ci and if I read the code correctly this is equivalent to -u all for test selection. However... the M1 buildbot runs macOS 14 which doesn't have this particular bug in the system install of zlib (and I also don't know if that system uses the system install of zlib or one installed through homebrew)

@ronaldoussoren
Copy link
Contributor

Is there anything left to do for this issue?

@ned-deily
Copy link
Member

I verified that both test_binascii and test_zlib did indeed fail when run with regrtest bigmem option -M 6gb on an affected system (i.e. Apple Silicon Mac running macOS 12.6.1).

However, running with --slow-ci or -u all was not sufficient to exercise this case. Adding a -M with value greater than 4Gb was needed. So perhaps we should be adding that to at least some CI test and/or buildbots configurations?

@gpshead
Copy link
Member

gpshead commented Dec 6, 2023

Thanks for confirming. It's basically us happening to not have the "right" older version OS runners that exhibit the platform bug in the text matrix. Existing tests would've worked if that style of test had been run there. Good to know, but there's not anything we can do about this code wise.

@gpshead gpshead closed this as completed Dec 6, 2023
@ned-deily
Copy link
Member

It may be that we don't have a vulnerable system now in our test matrix but we did up until not many months ago. Shouldn't we be testing the bigger mem cases on most platforms?

aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
…API calls to 1gig (python#112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
…API calls to 1gig (python#112615)

Work around a macOS bug, limit zlib crc32 calls to 1GiB.

Without this, `zlib.crc32` and `binascii.crc32` could produce incorrect
results on multi-gigabyte inputs depending on the macOS version's Apple
supplied zlib implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-mac stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants