Feature request: Add "save to fd" function #7

New Issue

NRK · 2023-02-14T05:13:48-08:00

NRK commented

2023-02-14 05:13:48 -08:00

It would avoid some TOCTOU issues in scrot (https://github.com/resurrecting-open-source-projects/scrot/issues/215). And it looks like file handling got centralized recently for the savers (c3d5249) so shouldn't be difficult to add, I think.

It would avoid some TOCTOU issues in `scrot` (https://github.com/resurrecting-open-source-projects/scrot/issues/215). And it looks like file handling got centralized recently for the savers (c3d5249) so shouldn't be difficult to add, I think.

kw commented

2023-02-14 08:19:48 -08:00

Right, fine with me, will do.
However, for symmetry with imlib_load_fd() I think it would have to be save to fd like:
void imlib_save_image_fd(int fd, const char *file);

Right, fine with me, will do. However, for symmetry with imlib_load_fd() I think it would have to be save to fd like: void imlib_save_image_fd(int fd, const char *file);

NRK commented

2023-02-14 08:28:37 -08:00

Either FILE * or fd should work fine for scrot's purposes.

Either `FILE *` or `fd` should work fine for scrot's purposes.

guijan commented

2023-02-14 09:36:51 -08:00

The function should also communicate error like the _with_errno_return functions.
The others don't have a documented way to communicate the exact nature of errors, scrot already made the mistake of adding code that incorrectly pokes into imlib's implementation details to try to get errno values.

Following the style of other functions in imlib, the one scrot needs would be declared like this:

EAPI void imlib_save_image_fd_with_errno_return(int fd, int *error_return);

The function should also communicate error like the `_with_errno_return` functions. The others don't have a documented way to communicate the exact nature of errors, scrot already made the mistake of adding code that incorrectly pokes into imlib's implementation details to try to get errno values. Following the style of other functions in imlib, the one scrot needs would be declared like this: ```c EAPI void imlib_save_image_fd_with_errno_return(int fd, int *error_return); ```

kw commented

2023-02-14 10:15:06 -08:00

At some point I thought I was going in that direction - having functions with and without error returns.
However, that turned out to become rather messy, IMO.
Instead (although not documented too well), the last error from any load (immediate) or save operation can be fetched with imlib_get_error() (as of v1.10.0).
As of the next release also errors from implicit deferred data loads can be fetched.

I intend to keep the file name parameter, again for symmetry with imlib_load_image_fd().

At some point I thought I was going in that direction - having functions with and without error returns. However, that turned out to become rather messy, IMO. Instead (although not documented too well), the last error from any load (immediate) or save operation can be fetched with imlib_get_error() (as of v1.10.0). As of the next release also errors from implicit deferred data loads can be fetched. I intend to keep the file name parameter, again for symmetry with imlib_load_image_fd().

guijan commented

2023-02-14 11:23:18 -08:00

Ideally, the library would only have functions with informative error return values. Programs that don't care about the exact nature of the error can simply check that the function didn't succeed and not do anything about the information contained in the error code.

A filename parameter would make imlib_save_image_fd useless, we're back to TOCTTOU baked into the API. It doesn't make sense to do this either, fds don't even necessarily have filenames. There are pipes, sockets, and so on, but even a fd that is a regular file doesn't need to have a name:

#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

int
main(void)
{
	int fd;
	const char msg[] = "This file has no name.";
	char readmsg[sizeof(msg)];
	char filename[] = "/tmp/noname.XXXXXXX";

	/* Create a file. */
	if ((fd = mkstemp(filename)) == -1)
		err(1, "mkstemp");
	/* Unlink the file. The file still exists, but it has no name in the
	 * filesystem, and will be garbage collected when the fd closes.
	 */
	if (unlink(filename) == -1)
		err(1, "unlink");

	/* Write a message to the file with no filename. */
	if (write(fd, msg, sizeof(msg)) != sizeof(msg))
		err(1, "write");
	/* Read back the message from the same file. */
	lseek(fd, 0, SEEK_SET);
	if (read(fd, readmsg, sizeof(readmsg)) != sizeof(readmsg))
		err(1, "read");

	close(fd);

	/* Print the message we got from the file. */
	if (puts(readmsg) == EOF)
		err(1, "puts");
}

Ideally, the library would only have functions with informative error return values. Programs that don't care about the exact nature of the error can simply check that the function didn't succeed and not do anything about the information contained in the error code. A filename parameter would make imlib_save_image_fd useless, we're back to TOCTTOU baked into the API. It doesn't make sense to do this either, fds don't even necessarily have filenames. There are pipes, sockets, and so on, but even a fd that is a regular file doesn't need to have a name: ```c #include <err.h> #include <fcntl.h> #include <stdio.h> #include <string.h> #include <unistd.h> int main(void) { int fd; const char msg[] = "This file has no name."; char readmsg[sizeof(msg)]; char filename[] = "/tmp/noname.XXXXXXX"; /* Create a file. */ if ((fd = mkstemp(filename)) == -1) err(1, "mkstemp"); /* Unlink the file. The file still exists, but it has no name in the * filesystem, and will be garbage collected when the fd closes. */ if (unlink(filename) == -1) err(1, "unlink"); /* Write a message to the file with no filename. */ if (write(fd, msg, sizeof(msg)) != sizeof(msg)) err(1, "write"); /* Read back the message from the same file. */ lseek(fd, 0, SEEK_SET); if (read(fd, readmsg, sizeof(readmsg)) != sizeof(readmsg)) err(1, "read"); close(fd); /* Print the message we got from the file. */ if (puts(readmsg) == EOF) err(1, "puts"); } ```

NRK commented

2023-02-14 11:30:53 -08:00

A filename parameter would make imlib_save_image_fd useless, we're back to TOCTTOU baked into the API.

I don't think so. The "filename" in load_fd is only used as an extension guess. The same is probably going to be true for save_fd. E.g imlib_save_image_fd(fd, ".png");

> A filename parameter would make imlib_save_image_fd useless, we're back to TOCTTOU baked into the API. I don't think so. The "filename" in `load_fd` is only used as an extension guess. The same is probably going to be true for `save_fd`. E.g `imlib_save_image_fd(fd, ".png");`

NRK changed title from ~~Feature request: Add "save to FILE *" function~~ to Feature request: Add "save to fd" function

2023-02-14 12:01:32 -08:00

kw referenced this issue from a commit

2023-02-15 09:50:04 -08:00

Add imlib_save_image_fd()

kw commented

2023-02-15 10:04:16 -08:00

Added now.
And yes, the file parameter is only used to determine the output file format if it isn't already set in the image (e.g. using imlib_image_set_format()).

Added now. And yes, the file parameter is only used to determine the output file format if it isn't already set in the image (e.g. using imlib_image_set_format()).

guijan commented

2023-02-15 10:58:25 -08:00

scrot doesn't care so this issue is solved, but does it really have to close the fd?

guijan commented

2023-02-15 11:04:52 -08:00

Wait, I just noticed.
ce0ac1eeff/src/lib/api.c (L1940)
stdout could be the same file as the fd. A library shouldn't produce output like this.

Wait, I just noticed. https://git.enlightenment.org/old/legacy-imlib2/src/commit/ce0ac1eeff8223447fee137b288b7e5b0668007f/src/lib/api.c#L1940 stdout could be the same file as the fd. A library shouldn't produce output like this.

kw commented

2023-02-15 12:31:08 -08:00

Right, forgot to clean out a bit of debug.

The fd is closed because load_fd does so.
load_fd() does so because at the time it was argued that it was the right thing to do, as various other image loading APIs do so.
But yeah, it still seems somewhat strange to me as well.

Right, forgot to clean out a bit of debug. The fd is closed because load_fd does so. load_fd() does so because at the time it was argued that it was the right thing to do, as various other image loading APIs do so. But yeah, it still seems somewhat strange to me as well.

NRK commented

2023-02-16 00:37:53 -08:00

This looks about right. So closing the issue.

Also - closing the fd seems dubious to me as well. But better to be consistent, I think. The caller just needs to dup the fd before passing it to imlib2.

This looks about right. So closing the issue. Also - closing the fd seems dubious to me as well. But better to be consistent, I think. The caller just needs to dup the fd before passing it to imlib2.

NRK closed this issue

2023-02-16 00:39:36 -08:00

Sign in to join this conversation.

No Label

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: old/legacy-imlib2#7