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

fix some int32 limits on windows #3935

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jcupitt
Copy link
Member

@jcupitt jcupitt commented Apr 16, 2024

write() on windows uses int for byte counts, leading to errors with very wide (more than 1.5m pixels wide) images.

libvips was assuming posix:

ssize_t write(int fd, const void buf[.count], size_t count);

But windows has:

int _write(int fd, const void *buffer, unsigned int count);

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/write?view=msvc-170

We probably have some other int32 limits still :( we should make some test cases and do more investigation, especially on windows.

Referring discussion:

#3918 (comment)

write() on windows uses int for byte counts, leading to errors with very
wide (more than 1.5m pixels wide) images
@jcupitt
Copy link
Member Author

jcupitt commented Apr 16, 2024

size_t is always correct for the size of memory objects, but it's 32-bits on win32 32-bit builds, which means it's too small for the size of filesystem objects.

size_t is 64 bits on 32-bit *nix systems as long as large file support is enabled (I think it always is).

Perhaps we should just use gint64 for all functions related to the filesystem? It'd be (maybe?) less confusing.

@kleisauke
Copy link
Member

This looks good! I'll test this out on Windows later.

We probably have some other int32 limits still

I think clang-tidy's cppcoreguidelines-narrowing-conversions check might catch such things. Here's the output of that check over the entire project:
https://gist.github.com/kleisauke/9025c3564d3aec17496bd21d675908fb

Though, that could also generate false-positives, for example:

char compression_string[2] = { '0' + compression, 0 };

../libvips/foreign/archive.c:180:33: warning: narrowing conversion from 'int' to signed type 'char' is implementation-defined [cppcoreguidelines-narrowing-conversions]
  180 |         char compression_string[2] = { '0' + compression, 0 };
      |                                        ^

compression is already limited to [0,9] when it reaches there.

@kleisauke
Copy link
Member

It looks like vipssave uses write_vips in generate.c instead.

/* A write function for VIPS images. Just write() the pixel data.
*/
static int
write_vips(VipsRegion *region, VipsRect *area, void *a)

Applying the same fix on that function (see e.g. commit kleisauke@af7c7bc) fixes the test case mentioned in #3918 (reply in thread) on my Windows 11 PC.

$ vips black x.v 1400000 10000 --bands 3 --vips-progress
vips x.v: 1400000 x 10000 pixels, 24 threads, 1400000 x 1 tiles, 512 lines in buffer
vips x.v: done in 18.5s
$ (Get-Item x.v).Length
42000002024

BTW, should this PR target the 8.15 branch instead?

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.

None yet

2 participants