BLI_fileops: Add 'BLI_read' wrapper to ensure the requested data is read #113474

Campbell Barton · 2023-10-10T08:04:41+02:00

Campbell Barton commented

2023-10-10 08:04:41 +02:00

According to the documentation read isn't required to read all the data requested. Although until recently I hadn't encountered this and none of Blender's code checks for read succeeding but not reading the requested size.

Resolves #113473 where images over 2gb fail to load.

It's surprising to me that this issue hasn't come up earlier, as far as I can tell - technically read can't be relied on to return the requested size, however in practice this almost never happens.

Regarding when to use this function, I'd suggest to do one of the following:

Use BLI_read everywhere (replacing all calls to read in source/), as the overhead is negligible.
Use BLI_read for all cases the size is unknown ahead of time, if a function uses read to load a fixed number of bytes (an image header for e.g.) leave this using read. For clarity name this BLI_read_with_large_file_support or similar.

Note this this isn't a problem for fread.

According to the documentation `read` isn't required to read all the data requested. Although until recently I hadn't encountered this and none of Blender's code checks for read succeeding but not reading the requested size. Resolves #113473 where images over 2gb fail to load. ---- It's surprising to me that this issue hasn't come up earlier, as far as I can tell - technically `read` can't be relied on to return the requested size, however in practice this almost never happens. ---- Regarding when to use this function, I'd suggest to do one of the following: - Use `BLI_read` *everywhere* (replacing all calls to `read` in `source/`), as the overhead is negligible. - Use `BLI_read` for all cases the size is unknown ahead of time, if a function uses read to load a fixed number of bytes (an image header for e.g.) leave this using `read`. For clarity name this `BLI_read_with_large_file_support` or similar. Note this this isn't a problem for `fread`.

Campbell Barton force-pushed pr-read-lfs from 0def1d40f5 to de6bfabe8d

2023-10-10 08:14:55 +02:00

Compare

Campbell Barton requested review from Brecht Van Lommel 2023-10-10 08:26:19 +02:00

Campbell Barton requested review from Sergey Sharybin 2023-10-10 08:26:36 +02:00

Campbell Barton requested review from Bastien Montagne 2023-10-10 08:32:34 +02:00

Bastien Montagne approved these changes 2023-10-10 09:50:55 +02:00

Bastien Montagne left a comment

Fairly surprising behavior from read indeed!

Patch LGTM, would suggest to double-check its impact on e.g. big blendfile reading, but would not expect any significant impact on performances either. If everything is fine, would rather replace all our usages of read by the new BLI_read then.

Fairly surprising behavior from `read` indeed! Patch LGTM, would suggest to double-check its impact on e.g. big blendfile reading, but would not expect any significant impact on performances either. If everything is fine, would rather replace all our usages of `read` by the new `BLI_read` then.

Brecht Van Lommel requested changes 2023-10-10 12:46:32 +02:00

Brecht Van Lommel left a comment

I think it's fine to use this everywhere to replace read.

However on Windows it will still fail to read files > 2GB. There read is an alias for _read, which uses int instead of ssize_t.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170

It could be fixed by doing something like this on Windows: read(fd, buf, min(nbytes, INT_MAX))

I think it's fine to use this everywhere to replace `read`. However on Windows it will still fail to read files > 2GB. There `read` is an alias for `_read`, which uses `int` instead of `ssize_t`. https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170 It could be fixed by doing something like this on Windows: `read(fd, buf, min(nbytes, INT_MAX))`

Campbell Barton force-pushed pr-read-lfs from de6bfabe8d to bf6efb1c25

2023-10-10 13:10:21 +02:00

Compare

Campbell Barton requested review from Brecht Van Lommel 2023-10-10 13:10:35 +02:00

Campbell Barton commented

2023-10-10 13:11:11 +02:00

Interesting, I wasn't aware of this read limitation on WIN32, updated the patch.

Interesting, I wasn't aware of this `read` limitation on WIN32, updated the patch.

Campbell Barton force-pushed pr-read-lfs from bf6efb1c25 to 8f893b91a5

2023-10-10 13:17:37 +02:00

Compare

Brecht Van Lommel requested changes 2023-10-10 13:19:14 +02:00

source/blender/blenlib/intern/fileops.c Outdated

						
				@ -95,0 +104,4 @@

				  while (true) {

				    ssize_t nbytes_read = read(fd,

				                               buf,

				#ifndef WIN32

Brecht Van Lommel commented

2023-10-10 13:19:00 +02:00

ifndef -> ifdef

`ifndef` -> `ifdef`

ideasman42 marked this conversation as resolved

Campbell Barton added 1 commit 2023-10-10 13:22:55 +02:00

47d2618bdf Correct ifdef

Campbell Barton requested review from Brecht Van Lommel 2023-10-10 13:23:02 +02:00

Brecht Van Lommel approved these changes 2023-10-10 13:23:28 +02:00

Campbell Barton referenced this issue from a commit

2023-10-10 13:28:42 +02:00

BLI_fileops: Add 'BLI_read' wrapper to ensure the requested data is read

Campbell Barton manually merged commit fa40724a72 into main

2023-10-10 13:53:48 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

BLI_fileops: Add 'BLI_read' wrapper to ensure the requested data is read #113474