Skip to content

Conversation

@tdp2110
Copy link

@tdp2110 tdp2110 commented Aug 8, 2022

When nels = 0, no bytes from const T* data are written to the npy
file (line 219: fwrite(data,sizeof(T),nels,fp)).
Therefore, the crc of the to-be-written data only needs to be
computed for the numpy header, as done on line 171.

It might be thought that crc32(initial_crc, data_ptr, data_size)
would always return initial_crc if data_size = 0. This is true
for every value of data_ptr except NULL. In this case crc32
returns 0, independent of initial_crc.

The result of calling npz_save with a shape of total size 0
(i.e., the product of all elements in the shape) should not depend
on whether data is a null pointer or not.

Fix this bug by only updating the crc when nels != 0, independent
of the value of data.

When nels = 0, no bytes from `const T* data` are written to the npy
file (line 219: fwrite(data,sizeof(T),nels,fp)).
Therefore, the crc of the to-be-written data only needs to be
computed for the numpy header, as done on line 171.

It might be thought that `crc32(initial_crc, data_ptr, data_size)`
would always return `initial_crc` if `data_size = 0`. This is true
for every value of data_ptr *except* NULL. In this case crc32
returns 0, *independent* of initial_crc.

The result of calling npz_save with a shape of total size 0
(i.e., the product of all elements in the shape) should not depend
on whether `data` is a null pointer or not.

Fix this bug by only updating the crc when nels != 0, independent
of the value of `data`.
@thomas-peters-cerebras
Copy link

Reference for crc32: https://refspecs.linuxbase.org/LSB_3.0.0/LSB-Core-generic/LSB-Core-generic/zlib-crc32-1.html. In the Description section, "initial checksum" refers to zero. See the source code (https://github.com/madler/zlib/blob/master/crc32.c#L626)

pkestene added a commit to pkestene/cnpy-cmake that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants