C file reading is implemented incorrectly

[nh2]

The project does not check for short read()s.

In C, as per man 2 read, it returns the number of Bytes read:

It is not an error if this number is smaller than the number of bytes requested; this may happen for example because fewer bytes are actually available right now

Thus you need to loop around read(), always.

Relevant code

i3status/src/general.c

Lines 34 to 37 in 200fef9

	/* We need one byte for the trailing 0 byte */
	int n = read(fd, destination, size - 1);
	if (n != -1)
	destination[n] = '\0';

i3status/src/print_file_contents.c

Line 36 in 200fef9

n = read(fd, buf, ctx->max_chars);

This is wrong since 15 years ago:

• https://github.com/i3/i3status/blame/34ba9fa9083c655e5ba06cecd46da157eb07d980/src/general.c#L18-L21
• https://github.com/i3/i3status/blame/ebfafc5dac9cb15ec4ec9b13e9b933c9c2c5d5c9/src/general.c#L22-L24

[orestisfl]

Thus you need to loop around read(), always.

I am not so sure. In i3status, it's usually better to keep going to not block the program rather than potentially being stuck in a while-read loop.

[stapelberg]

The reason why the lack of short read handling is typically not a problem with i3status is that it mostly reads /proc and /sys files, for which the kernel never returns a short read.

That said, at the very least for print_file_contents.c we should consider handling short reads, and it probably doesn’t hurt for the other reads either.

@orestisfl’s point also stands, though: would handling short reads change behavior when i3status is configured to print a file that is backed by an unavailable NFS mount (for example)?

[nh2]

In i3status, it's usually better to keep going to not block the program rather than potentially being stuck in a while-read loop.

Short reads can be very confusing and lead to bad decisions. Imagine that this very general function slurp() one day gets used to read a path to delete stored on disk in some config file ending in cacheDir = /home/myuser/.cache, short-reads only cacheDir = /home/myuser, and then runs rm -r on that.

The reason why the lack of short read handling is typically not a problem with i3status is that it mostly reads /proc and /sys files, for which the kernel never returns a short read.

Fair point. Then the function should have a mega disclaimer at the top though when it's safe to use, probably even in the name.

That said, at the very least for print_file_contents.c we should consider handling short reads, and it probably doesn’t hurt for the other reads either.

Yes, I think that's better. If some file by implementation of the kernel needs only one call to read, that's nice and fast, but that shouldn't be assumed in general-purpose functions because it's easy to introduce future bugs.

would handling short reads change behavior when i3status is configured to print a file that is backed by an unavailable NFS mount (for example)?

I think in the general case on a hanging kernel NFS, or FUSE mount, already the first read() will hang indefinitely (or for very long), so I doubt short-reading it would help here.

The only situation I can imagine (but am not sure it actually is implemented that way) is that you read a very large file (e.g. 1 GB), and after the first 500 MB the NFS server goes down (and clearly says so, thus avoiding a hang). Maybe then you get s short read.

---

From what I can tell so far, short reads don't seem to be designed to handle hangs from userspace, but to allow kernels to work with non-infinite buffers.

[stapelberg]

Imagine that this very general function slurp() one day gets used to read a path to delete stored on disk in some config file ending in cacheDir = /home/myuser/.cache, short-reads only cacheDir = /home/myuser, and then runs rm -r on that.

As a matter of policy, i3status must never intentionally modify system state, including writing new files or deleting existing files.

I think the potential for short reads to cause issues is limited to “my i3status briefly showed a cut-off value”.

But, I also don’t want to keep arguing over this detail. Do you want to send a PR to add the loop?