diff mbox series

[2/3] handlers: add pipe handler

Message ID 20210305053935.1786812-3-dominique.martinet@atmark-techno.com
State Changes Requested
Headers show
Series add pipe handler | expand

Commit Message

Dominique Martinet March 5, 2021, 5:39 a.m. UTC
The pipe handler can be used to spawn an arbitrary command and
feed a file or image to its stdin.

This can be used for example to load a container image embedded
in the swu or similar process.

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
Link: https://groups.google.com/g/swupdate/c/DMc8vNCQdLI/m/yaFMldRkAAAJ
---
 configs/all_handlers_defconfig |   1 +
 doc/source/handlers.rst        |  25 ++++
 handlers/Config.in             |   7 +
 handlers/Makefile              |   1 +
 handlers/pipe_handler.c        | 227 +++++++++++++++++++++++++++++++++
 5 files changed, 261 insertions(+)
 create mode 100644 handlers/pipe_handler.c

Comments

Dominique Martinet March 5, 2021, 5:49 a.m. UTC | #1
Dominique Martinet wrote on Fri, Mar 05, 2021 at 02:39:34PM +0900:
> +	/* pipe data to process. Ignoring sigpipe lets the handler error
> +	 * properly when writing to a broken pipe instead of exiting */
> +	signal(SIGPIPE, SIG_IGN);

Had forgotten this bit: I never restore the signal handler after
copyimage.
I should either restore it or we should ignore SIGPIPE as part
of swupdate_init to have a consistent behaviour, I'd honestly rather do
the later as I don't think it's ever good to have swupdate killed by
SIGPIPE without any error message.

In the current code if we juts ignore the signal the errors do bubble up
quite nicely, so I don't expect any problem if we just ignore it
everywhere, but I'm not aware of how exactly all the handlers work.
Stefano Babic March 7, 2021, 12:50 p.m. UTC | #2
Hi Dominique,

On 05.03.21 06:39, Dominique Martinet wrote:
> The pipe handler can be used to spawn an arbitrary command and
> feed a file or image to its stdin.
> 
> This can be used for example to load a container image embedded
> in the swu or similar process.

I have some general remark:

- there are already global function for spawning processes, see 
spawn_process (now static). Can we maybe reuse some code ?
- As we start an external command / process, it will be nice to add 
run_as_userid and run_as_groupid properties, with default to uid and gid 
of the handler.

> 
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Link: https://groups.google.com/g/swupdate/c/DMc8vNCQdLI/m/yaFMldRkAAAJ
> ---
>   configs/all_handlers_defconfig |   1 +
>   doc/source/handlers.rst        |  25 ++++
>   handlers/Config.in             |   7 +
>   handlers/Makefile              |   1 +
>   handlers/pipe_handler.c        | 227 +++++++++++++++++++++++++++++++++
>   5 files changed, 261 insertions(+)
>   create mode 100644 handlers/pipe_handler.c
> 
> diff --git a/configs/all_handlers_defconfig b/configs/all_handlers_defconfig
> index 426026701728..8f374850f0b9 100644
> --- a/configs/all_handlers_defconfig
> +++ b/configs/all_handlers_defconfig
> @@ -24,3 +24,4 @@ CONFIG_SWUFORWARDER_HANDLER=y
>   CONFIG_BOOTLOADERHANDLER=y
>   CONFIG_SSBLSWITCH=y
>   CONFIG_UCFWHANDLER=y
> +CONFIG_PIPEHANDLER=y
> diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
> index 07fb6c2efb78..764e0d0d09a2 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -857,3 +857,28 @@ found on the device. It is a partition handler and it runs before any image is i
>                                      "18e12df1-d8e1-4283-8727-37727eb4261d"];
>   		}
>   	});
> +
> +Pipe Handler
> +------------
> +
> +This handler spawns an arbitrary command and feed the file or image data to it.
> +
> +Note that if installed-directly is passed, the checksum is validated as data
> +is piped to the command which runs until the end, so if the checksum fails
> +the harm has still been done and it is not recommended to modify data in place
> +that way.
> +
> +
> +::
> +
> +	images: (
> +		{
> +			type = "pipe";
> +			filename = "container_image.tar";
> +			sha256 = "c63cc116ed10eb03153d0a76cb59555d524fe89484bc3b9d81267014f8170e8e";
> +			installed-directly = true;
> +			properties: {
> +				cmd: "podman load";
> +			}
> +		}
> +	);
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 8571c0c10d19..d2553428fa21 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -275,6 +275,13 @@ config UCFWHANDLER
>   	  Simple protocol to upgrade a microcontroller
>   	  via UART.
>   
> +config PIPEHANDLER
> +	bool "pipe handler"
> +	default n
> +	help
> +	  This handler forks an arbitrary command and pipes the
> +	  cpio file to it.
> +
>   comment "Microcontroller handler depends on libgpiod"
>   	depends on !HAVE_LIBGPIOD
>   
> diff --git a/handlers/Makefile b/handlers/Makefile
> index 14491391186c..92ba0352da1f 100644
> --- a/handlers/Makefile
> +++ b/handlers/Makefile
> @@ -24,3 +24,4 @@ obj-$(CONFIG_SSBLSWITCH) += ssbl_handler.o
>   obj-$(CONFIG_SWUFORWARDER_HANDLER) += swuforward_handler.o swuforward-ws.o
>   obj-$(CONFIG_UBIVOL)	+= ubivol_handler.o
>   obj-$(CONFIG_UCFWHANDLER)	+= ucfw_handler.o
> +obj-$(CONFIG_PIPEHANDLER)	+= pipe_handler.o
> diff --git a/handlers/pipe_handler.c b/handlers/pipe_handler.c
> new file mode 100644
> index 000000000000..5c6365614b5c
> --- /dev/null
> +++ b/handlers/pipe_handler.c
> @@ -0,0 +1,227 @@
> +/*
> + * (C) Copyright 2021
> + * Dominique Martinet, Atmark Techno, dominique.martinet@atmark-techno.com
> + *
> + * SPDX-License-Identifier:     GPL-2.0-or-later
> + */
> +
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +
> +#include "swupdate.h"
> +#include "handler.h"
> +#include "util.h"
> +
> +struct pipe_priv {
> +	pid_t pid;
> +	/* pipes from the point of view of the spawned process */
> +	int stdin;
> +	int stdout;
> +	int stderr;
> +	int status;
> +
> +	char stdout_buf[SWUPDATE_GENERAL_STRING_SIZE];
> +	int stdout_index;
> +	char stderr_buf[SWUPDATE_GENERAL_STRING_SIZE];
> +	int stderr_index;
> +};
> +
> +/* This helper polls a process stdout/stderr and forwards these to
> + * TRACE/ERROR appropriately.
> + * It stops when there is nothing left on stdout/stderr if the
> + * process terminated or it was requested to write and stdin is writable
> + */
> +static int pipe_poll_process(struct pipe_priv *priv, int write)
> +{
> +	int ret = 0;
> +	int wstatus;
> +
> +	while (1) {
> +		fd_set readfds;
> +		fd_set writefds;
> +		struct timeval tv = { .tv_sec = 1, };
> +		int max_fd;
> +		int n = 0;
> +
> +		FD_ZERO(&readfds);
> +		FD_ZERO(&writefds);
> +		FD_SET(priv->stdout, &readfds);
> +		FD_SET(priv->stderr, &readfds);
> +		max_fd = max(priv->stdout, priv->stderr);
> +		if (write) {
> +			FD_SET(priv->stdin, &writefds);
> +			max_fd = max(max_fd, priv->stdin);
> +		}
> +
> +		ret = select(max_fd + 1, &readfds, &writefds, NULL, &tv);
> +		if (ret < 0) {
> +			ret = -errno;
> +			ERROR("select failed: %d\n", -ret);
> +			return ret;
> +		}
> +		if (FD_ISSET(priv->stdout, &readfds)) {
> +			ret = read_lines_notify(priv->stdout, priv->stdout_buf,
> +						SWUPDATE_GENERAL_STRING_SIZE,
> +						&priv->stdout_index, TRACELEVEL);
> +			if (ret < 0) {
> +				ERROR("Could not read stdout: %d\n", ret);
> +				return ret;
> +			}
> +			n += ret;
> +		}
> +		if (FD_ISSET(priv->stderr, &readfds)) {
> +			ret = read_lines_notify(priv->stderr, priv->stderr_buf,
> +						SWUPDATE_GENERAL_STRING_SIZE,
> +						&priv->stderr_index, ERRORLEVEL);
> +			if (ret < 0) {
> +				ERROR("Could not read stderr: %d\n", ret);
> +				return ret;
> +			}
> +			n += ret;
> +		}
> +		/* keep reading from stdout/stderr if there was anything */
> +		if (n > 0)
> +			continue;
> +
> +		/* return if process exited */
> +		pid_t w = waitpid(priv->pid, &wstatus, WNOHANG);
> +		if (w < 0) {
> +			ret = -errno;
> +			ERROR("Could not waitpid: %d", -ret);
> +			return ret;
> +		}
> +		if (w == priv->pid) {
> +			if (WIFEXITED(wstatus)) {
> +				priv->status = WEXITSTATUS(wstatus);
> +				ret = -priv->status;
> +				TRACE("Command returned %d", -ret);
> +			} else if (WIFSIGNALED(wstatus)) {
> +				priv->status = 1;
> +				ret = -1;
> +				TRACE("Command killed by signal %d",
> +				      WTERMSIG(wstatus));
> +			} else {
> +				priv->status = 1;
> +				ERROR("wait returned but no exit code nor signal?");
> +				ret = -1;
> +			}
> +			return ret;
> +		}
> +
> +		/* or if we can write */
> +		if (write && FD_ISSET(priv->stdin, &writefds))
> +			return 0;
> +	}
> +}
> +
> +static int pipe_copy_callback(void *out, const void *buf, unsigned int len)
> +{
> +	struct pipe_priv *priv = out;
> +	int ret;
> +
> +	/* check data from subprocess */
> +	ret = pipe_poll_process(priv, 1);

I am not sure if this works in any conditions. This relies on the fact 
that the callback is always called, but if the copyimage() finds an 
error, for example in case of compressed or encrypted data, this is not 
called anymore letting the process in an unknow status (but process 
should see that the stdin is closed) and further messages are not read 
again because this callback (running in context of the handler) is not 
called anymore. IMHO this can be solved dropping the callback (not 
needed because the pipe can be passed as output file) and creating a 
thread (see other handlers) with the goal to read stdout / stderr.

> +	if (ret < 0)
> +		return ret;
> +
> +	/* let copy_write do the actual copying */
> +	return copy_write(&priv->stdin, buf, len);
> +}
> +
> +static int pipe_image(struct img_type *img,
> +	void __attribute__ ((__unused__)) *data)
> +{
> +	struct pipe_priv priv = { .status = -1, };
> +	int ret, pollret;
> +	int stdinpipe[2], stdoutpipe[2], stderrpipe[2];
> +	char *cmd = dict_get_value(&img->properties, "cmd");
> +	if (!cmd) {
> +		ERROR("Pipe handler needs a command to pipe data into: please set the 'cmd' property");
> +		return -EINVAL;
> +	}
> +
> +	if (pipe(stdinpipe) < 0) {
> +		ERROR("Could not create process pipes");
> +		return -EFAULT;
> +	}
> +	if (pipe(stdoutpipe) < 0) {
> +		ERROR("Could not create process pipes");
> +		close(stdinpipe[0]);
> +		close(stdinpipe[1]);
> +		return -EFAULT;
> +	}
> +	if (pipe(stderrpipe) < 0) {
> +		ERROR("Could not create process pipes");
> +		close(stdinpipe[0]);
> +		close(stdinpipe[1]);
> +		close(stdoutpipe[0]);
> +		close(stdoutpipe[1]);
> +		return -EFAULT;
> +	}
> +
> +	priv.pid = fork();
> +	if (priv.pid == 0) {
> +		/* child process: cleanup fds and exec */
> +		if (dup2(stdinpipe[0], STDIN_FILENO) < 0)
> +			exit(1);
> +		if (dup2(stdoutpipe[1], STDOUT_FILENO) < 0)
> +			exit(1);
> +		if (dup2(stderrpipe[1], STDERR_FILENO) < 0)
> +			exit(1);
> +		close(stdinpipe[0]);
> +		close(stdinpipe[1]);
> +		close(stdoutpipe[0]);
> +		close(stdoutpipe[1]);
> +		close(stderrpipe[0]);
> +		close(stderrpipe[1]);
> +
> +		ret = execl("/bin/sh", "sh", "-c", cmd, NULL);
> +		ERROR("Cannot execute pipe handler: %s", strerror(errno));
> +		exit(1);
> +	}
> +
> +	/* parent process */
> +	close(stdinpipe[0]);
> +	close(stdoutpipe[1]);
> +	close(stderrpipe[1]);
> +	priv.stdin = stdinpipe[1];
> +	priv.stdout = stdoutpipe[0];
> +	priv.stderr = stderrpipe[0];
> +
> +	/* pipe data to process. Ignoring sigpipe lets the handler error
> +	 * properly when writing to a broken pipe instead of exiting */
> +	signal(SIGPIPE, SIG_IGN);
> +	ret = copyimage(&priv, img, pipe_copy_callback);
> +	if (ret < 0) {
> +		ERROR("Error copying data to pipe");
> +	}

My bigger concern is regarding the duplication of code between this 
handler and code in pctl.c. I would like to check with you which options 
we have before taking this code.


> +
> +	/* close stdin and keep reading process stdout/stderr until it exits
> +	 * (skip if already exited) */
> +	close(priv.stdin);
> +	if (priv.status == -1) {
> +		pollret = pipe_poll_process(&priv, 0);
> +		/* keep original error if we had any */
> +		if (!ret)
> +			ret = pollret;
> +	}
> +	close(priv.stdout);
> +	close(priv.stderr);
> +
> +	/* empty trailing buffers */
> +	if (priv.stdout_index)
> +		TRACE("%s", priv.stdout_buf);
> +	if (priv.stderr_index)
> +		ERROR("%s", priv.stderr_buf);
> +
> +	TRACE("finished piping image");
> +	return ret;
> +}
> +
> +__attribute__((constructor))
> +static void pipe_handler(void)
> +{
> +	register_handler("pipe", pipe_image,
> +			 IMAGE_HANDLER | FILE_HANDLER, NULL);
> +}
> 


Best regards,
Stefano Babic
Dominique Martinet March 8, 2021, 12:32 a.m. UTC | #3
Stefano Babic wrote on Sun, Mar 07, 2021 at 01:50:39PM +0100:
> > This can be used for example to load a container image embedded
> > in the swu or similar process.
> 
> I have some general remark:
> 
> - there are already global function for spawning processes, see
> spawn_process (now static). Can we maybe reuse some code ?

spawn_process looks pretty specific to swupdate-children (single pipe
communication, notify mechanism), and has nothing to control the child's
standard input/output/error fd so would be difficult to use as is.

I'm happy to add a different helper to share the pipe creations, fork
and exec with run_system_cmd but it requires quite a few parameters to
return (a fd per stream and child pid) so might not be easy to reuse in
practice

> - As we start an external command / process, it will be nice to add
> run_as_userid and run_as_groupid properties, with default to uid and gid of
> the handler.

I hadn't seen these properties, I agree they are worth adding; will add
an extra patch for v2.

> >
> > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> > Link: https://groups.google.com/g/swupdate/c/DMc8vNCQdLI/m/yaFMldRkAAAJ
> > ---
> > +static int pipe_copy_callback(void *out, const void *buf, unsigned int len)
> > +{
> > +	struct pipe_priv *priv = out;
> > +	int ret;
> > +
> > +	/* check data from subprocess */
> > +	ret = pipe_poll_process(priv, 1);
> 
> I am not sure if this works in any conditions. This relies on the fact that
> the callback is always called, but if the copyimage() finds an error, for
> example in case of compressed or encrypted data, this is not called anymore
> letting the process in an unknow status (but process should see that the
> stdin is closed) and further messages are not read again because this
> callback (running in context of the handler) is not called anymore. IMHO
> this can be solved dropping the callback (not needed because the pipe can be
> passed as output file) and creating a thread (see other handlers) with the
> goal to read stdout / stderr.

The reason I made pipe_poll_process a separate function is that it is
also called once after the copy has finished to properly finish reading
anything the process has left to say and reap it.

I did test the various cases (child process outputing something without
reading anything, before reading, after waiting for stdin to close)
without problem in this case.


I agree this could also have been achieved by another thread, and
actually did consider that first, but I generally prefer callbacks if it
can do the job -- please ask if you really prefer a threaded approach.


> > +	/* parent process */
> > +	close(stdinpipe[0]);
> > +	close(stdoutpipe[1]);
> > +	close(stderrpipe[1]);
> > +	priv.stdin = stdinpipe[1];
> > +	priv.stdout = stdoutpipe[0];
> > +	priv.stderr = stderrpipe[0];
> > +
> > +	/* pipe data to process. Ignoring sigpipe lets the handler error
> > +	 * properly when writing to a broken pipe instead of exiting */
> > +	signal(SIGPIPE, SIG_IGN);
> > +	ret = copyimage(&priv, img, pipe_copy_callback);
> > +	if (ret < 0) {
> > +		ERROR("Error copying data to pipe");
> > +	}
> 
> My bigger concern is regarding the duplication of code between this handler
> and code in pctl.c. I would like to check with you which options we have
> before taking this code.

The main reason I didn't make another subfunction for this part (up to
copyimage) was that the code is slightly different because of the number
of pipes, but that is admitedly easy enough to work.

The function would be a bit clumsy though, something like that?
(pseudocode)

int fork_cmd(char *cmd, pid_t *child_pid, int *child_stdin,
	     int *child_stdout, int *child_stderr)
{
	if (!child_pid)
		error
	if (child_stdin) {
		pipe for stdin
	}
	if (child_stdout) {
		pipe for stdout
	}
	if (child_stderr) {
		pipe for stderr
	}
	pid = fork()
	if (pid == 0) {
		dup fds
		exec
		error/exit
	}
	cleanup fds
	return
}

That could easily be shared with run_system_cmd
Stefano Babic March 8, 2021, 8:17 a.m. UTC | #4
Hi Dominique,

On 08.03.21 01:32, Dominique Martinet wrote:
> Stefano Babic wrote on Sun, Mar 07, 2021 at 01:50:39PM +0100:
>>> This can be used for example to load a container image embedded
>>> in the swu or similar process.
>>
>> I have some general remark:
>>
>> - there are already global function for spawning processes, see
>> spawn_process (now static). Can we maybe reuse some code ?
> 
> spawn_process looks pretty specific to swupdate-children (single pipe
> communication, notify mechanism),

Yes, it is. Its purpose is to spawn SWUpdate internal processes or 
external processes at startup.

> and has nothing to control the child's
> standard input/output/error fd so would be difficult to use as is.
> 
> I'm happy to add a different helper to share the pipe creations, fork
> and exec with run_system_cmd but it requires quite a few parameters to
> return (a fd per stream and child pid) so might not be easy to reuse in
> practice

Ok - let's start as you have done, a helper can be added if it will be 
really necessary. It does not make sense to add complexity when it is 
not yet necessary.

> 
>> - As we start an external command / process, it will be nice to add
>> run_as_userid and run_as_groupid properties, with default to uid and gid of
>> the handler.
> 
> I hadn't seen these properties, I agree they are worth adding; will add
> an extra patch for v2.

Fine.

> 
>>>
>>> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
>>> Link: https://groups.google.com/g/swupdate/c/DMc8vNCQdLI/m/yaFMldRkAAAJ
>>> ---
>>> +static int pipe_copy_callback(void *out, const void *buf, unsigned int len)
>>> +{
>>> +	struct pipe_priv *priv = out;
>>> +	int ret;
>>> +
>>> +	/* check data from subprocess */
>>> +	ret = pipe_poll_process(priv, 1);
>>
>> I am not sure if this works in any conditions. This relies on the fact that
>> the callback is always called, but if the copyimage() finds an error, for
>> example in case of compressed or encrypted data, this is not called anymore
>> letting the process in an unknow status (but process should see that the
>> stdin is closed) and further messages are not read again because this
>> callback (running in context of the handler) is not called anymore. IMHO
>> this can be solved dropping the callback (not needed because the pipe can be
>> passed as output file) and creating a thread (see other handlers) with the
>> goal to read stdout / stderr.
> 
> The reason I made pipe_poll_process a separate function is that it is
> also called once after the copy has finished to properly finish reading
> anything the process has left to say and reap it.

Yes, this is strongly necessary.

> 
> I did test the various cases (child process outputing something without
> reading anything, before reading, after waiting for stdin to close)
> without problem in this case.

ok, thanks for test all use cases.

> 
> 
> I agree this could also have been achieved by another thread, and
> actually did consider that first, but I generally prefer callbacks if it
> can do the job -- please ask if you really prefer a threaded approach.

Again, I raise concerns just reading the code but I have not yet tried 
to apply your patches and I haven't tested myself. If it works in any 
conditions withot additional threads, that is fine. In fact, reading the 
pipe after finishing should be enough.


> 
> 
>>> +	/* parent process */
>>> +	close(stdinpipe[0]);
>>> +	close(stdoutpipe[1]);
>>> +	close(stderrpipe[1]);
>>> +	priv.stdin = stdinpipe[1];
>>> +	priv.stdout = stdoutpipe[0];
>>> +	priv.stderr = stderrpipe[0];
>>> +
>>> +	/* pipe data to process. Ignoring sigpipe lets the handler error
>>> +	 * properly when writing to a broken pipe instead of exiting */
>>> +	signal(SIGPIPE, SIG_IGN);
>>> +	ret = copyimage(&priv, img, pipe_copy_callback);
>>> +	if (ret < 0) {
>>> +		ERROR("Error copying data to pipe");
>>> +	}
>>
>> My bigger concern is regarding the duplication of code between this handler
>> and code in pctl.c. I would like to check with you which options we have
>> before taking this code.
> 
> The main reason I didn't make another subfunction for this part (up to
> copyimage) was that the code is slightly different because of the number
> of pipes, but that is admitedly easy enough to work.
> 
> The function would be a bit clumsy though, something like that?
> (pseudocode)
> 
> int fork_cmd(char *cmd, pid_t *child_pid, int *child_stdin,
> 	     int *child_stdout, int *child_stderr)
> {
> 	if (!child_pid)
> 		error
> 	if (child_stdin) {
> 		pipe for stdin
> 	}
> 	if (child_stdout) {
> 		pipe for stdout
> 	}
> 	if (child_stderr) {
> 		pipe for stderr
> 	}
> 	pid = fork()
> 	if (pid == 0) {
> 		dup fds
> 		exec
> 		error/exit
> 	}
> 	cleanup fds
> 	return
> }
> 

Mmhh...it is not worth. Let this away. After adding your handler, I do 
not think there will be other different cases to run external commands.

> That could easily be shared with run_system_cmd
> 

Best regards,
Stefano Babic
Dominique Martinet March 9, 2021, 5:55 a.m. UTC | #5
Stefano Babic wrote on Mon, Mar 08, 2021 at 09:17:30AM +0100:
> > I agree this could also have been achieved by another thread, and
> > actually did consider that first, but I generally prefer callbacks if it
> > can do the job -- please ask if you really prefer a threaded approach.
> 
> Again, I raise concerns just reading the code but I have not yet tried to
> apply your patches and I haven't tested myself. If it works in any
> conditions withot additional threads, that is fine. In fact, reading the
> pipe after finishing should be enough.

If we do not handle read in the callback (or in another thread), it is
possible the forked program outputs a lot of text first and blocks 
on pipe write before reading what we want to pipe, deadlocking swupdate.

In general that should not happen but better be safe here.

(No worry about raising concerns early, please do; nothing is set in
stone yet and I have a few months ahead so no pressure to use something
fast)


>>> I noticed along the way that if installed-directly is set the checksum
>>> is validated while the file is streamed (obviously), so I documented
>>> that fact and the last patch adds a way to run an extra rollback
>>> command.
>>
>> I check this - this seems like a general feature more as a handler
>> specific one.
>
> I hadn't thought of it at the time but yes that is more general than
> what I made it to be, let's drop my third patch for now.

While I'm writing a wishlist to santa, now I've actually written a
"real" receive handler, what would be great would be the possibility to
set both a command on success and on failure (so the handler would be in
two stages: first stage receives the data in a safe area, but in final
and space-efficient way; second stage either swaps it active or cleans
it up)
I'm not sure if it's already possible to control the order in which
swupdate stages are run (does that depend on the order of files in the
cpio?) and if later steps are run if a previous one failed (I'd guess
no?), that could be enough although perhaps a bit fragile depending on
ordering method.


But since this is really just about the checksum failing I think it
would be cleaner to just optionally notify the child about it somehow --
for example optionally sending USR1 or USR2 after copyfile() depending
on its return code in the pipe handler?

The handler could then wait for either and decide what to do itself.
(that is possible in busybox sh with e.g. `trap "something" USR1`, where
something would be a function that sets a variable, and the main thread
of execution would loop on checking that variable & sleep (it needs to
be "moving" for the signal handler to be executed))


I guess that is a bit overengineered, but just enforcing run_as_*
don't feel enough in that case and I don't have much other ideas. Just
two commands would do, but I can imagine you not being thrilled about
the idea either.


Thanks,
diff mbox series

Patch

diff --git a/configs/all_handlers_defconfig b/configs/all_handlers_defconfig
index 426026701728..8f374850f0b9 100644
--- a/configs/all_handlers_defconfig
+++ b/configs/all_handlers_defconfig
@@ -24,3 +24,4 @@  CONFIG_SWUFORWARDER_HANDLER=y
 CONFIG_BOOTLOADERHANDLER=y
 CONFIG_SSBLSWITCH=y
 CONFIG_UCFWHANDLER=y
+CONFIG_PIPEHANDLER=y
diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
index 07fb6c2efb78..764e0d0d09a2 100644
--- a/doc/source/handlers.rst
+++ b/doc/source/handlers.rst
@@ -857,3 +857,28 @@  found on the device. It is a partition handler and it runs before any image is i
                                    "18e12df1-d8e1-4283-8727-37727eb4261d"];
 		}
 	});
+
+Pipe Handler
+------------
+
+This handler spawns an arbitrary command and feed the file or image data to it.
+
+Note that if installed-directly is passed, the checksum is validated as data
+is piped to the command which runs until the end, so if the checksum fails
+the harm has still been done and it is not recommended to modify data in place
+that way.
+
+
+::
+
+	images: (
+		{
+			type = "pipe";
+			filename = "container_image.tar";
+			sha256 = "c63cc116ed10eb03153d0a76cb59555d524fe89484bc3b9d81267014f8170e8e";
+			installed-directly = true;
+			properties: {
+				cmd: "podman load";
+			}
+		}
+	);
diff --git a/handlers/Config.in b/handlers/Config.in
index 8571c0c10d19..d2553428fa21 100644
--- a/handlers/Config.in
+++ b/handlers/Config.in
@@ -275,6 +275,13 @@  config UCFWHANDLER
 	  Simple protocol to upgrade a microcontroller
 	  via UART.
 
+config PIPEHANDLER
+	bool "pipe handler"
+	default n
+	help
+	  This handler forks an arbitrary command and pipes the
+	  cpio file to it.
+
 comment "Microcontroller handler depends on libgpiod"
 	depends on !HAVE_LIBGPIOD
 
diff --git a/handlers/Makefile b/handlers/Makefile
index 14491391186c..92ba0352da1f 100644
--- a/handlers/Makefile
+++ b/handlers/Makefile
@@ -24,3 +24,4 @@  obj-$(CONFIG_SSBLSWITCH) += ssbl_handler.o
 obj-$(CONFIG_SWUFORWARDER_HANDLER) += swuforward_handler.o swuforward-ws.o
 obj-$(CONFIG_UBIVOL)	+= ubivol_handler.o
 obj-$(CONFIG_UCFWHANDLER)	+= ucfw_handler.o
+obj-$(CONFIG_PIPEHANDLER)	+= pipe_handler.o
diff --git a/handlers/pipe_handler.c b/handlers/pipe_handler.c
new file mode 100644
index 000000000000..5c6365614b5c
--- /dev/null
+++ b/handlers/pipe_handler.c
@@ -0,0 +1,227 @@ 
+/*
+ * (C) Copyright 2021
+ * Dominique Martinet, Atmark Techno, dominique.martinet@atmark-techno.com
+ *
+ * SPDX-License-Identifier:     GPL-2.0-or-later
+ */
+
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "swupdate.h"
+#include "handler.h"
+#include "util.h"
+
+struct pipe_priv {
+	pid_t pid;
+	/* pipes from the point of view of the spawned process */
+	int stdin;
+	int stdout;
+	int stderr;
+	int status;
+
+	char stdout_buf[SWUPDATE_GENERAL_STRING_SIZE];
+	int stdout_index;
+	char stderr_buf[SWUPDATE_GENERAL_STRING_SIZE];
+	int stderr_index;
+};
+
+/* This helper polls a process stdout/stderr and forwards these to
+ * TRACE/ERROR appropriately.
+ * It stops when there is nothing left on stdout/stderr if the
+ * process terminated or it was requested to write and stdin is writable
+ */
+static int pipe_poll_process(struct pipe_priv *priv, int write)
+{
+	int ret = 0;
+	int wstatus;
+
+	while (1) {
+		fd_set readfds;
+		fd_set writefds;
+		struct timeval tv = { .tv_sec = 1, };
+		int max_fd;
+		int n = 0;
+
+		FD_ZERO(&readfds);
+		FD_ZERO(&writefds);
+		FD_SET(priv->stdout, &readfds);
+		FD_SET(priv->stderr, &readfds);
+		max_fd = max(priv->stdout, priv->stderr);
+		if (write) {
+			FD_SET(priv->stdin, &writefds);
+			max_fd = max(max_fd, priv->stdin);
+		}
+
+		ret = select(max_fd + 1, &readfds, &writefds, NULL, &tv);
+		if (ret < 0) {
+			ret = -errno;
+			ERROR("select failed: %d\n", -ret);
+			return ret;
+		}
+		if (FD_ISSET(priv->stdout, &readfds)) {
+			ret = read_lines_notify(priv->stdout, priv->stdout_buf,
+						SWUPDATE_GENERAL_STRING_SIZE,
+						&priv->stdout_index, TRACELEVEL);
+			if (ret < 0) {
+				ERROR("Could not read stdout: %d\n", ret);
+				return ret;
+			}
+			n += ret;
+		}
+		if (FD_ISSET(priv->stderr, &readfds)) {
+			ret = read_lines_notify(priv->stderr, priv->stderr_buf,
+						SWUPDATE_GENERAL_STRING_SIZE,
+						&priv->stderr_index, ERRORLEVEL);
+			if (ret < 0) {
+				ERROR("Could not read stderr: %d\n", ret);
+				return ret;
+			}
+			n += ret;
+		}
+		/* keep reading from stdout/stderr if there was anything */
+		if (n > 0)
+			continue;
+
+		/* return if process exited */
+		pid_t w = waitpid(priv->pid, &wstatus, WNOHANG);
+		if (w < 0) {
+			ret = -errno;
+			ERROR("Could not waitpid: %d", -ret);
+			return ret;
+		}
+		if (w == priv->pid) {
+			if (WIFEXITED(wstatus)) {
+				priv->status = WEXITSTATUS(wstatus);
+				ret = -priv->status;
+				TRACE("Command returned %d", -ret);
+			} else if (WIFSIGNALED(wstatus)) {
+				priv->status = 1;
+				ret = -1;
+				TRACE("Command killed by signal %d",
+				      WTERMSIG(wstatus));
+			} else {
+				priv->status = 1;
+				ERROR("wait returned but no exit code nor signal?");
+				ret = -1;
+			}
+			return ret;
+		}
+
+		/* or if we can write */
+		if (write && FD_ISSET(priv->stdin, &writefds))
+			return 0;
+	}
+}
+
+static int pipe_copy_callback(void *out, const void *buf, unsigned int len)
+{
+	struct pipe_priv *priv = out;
+	int ret;
+
+	/* check data from subprocess */
+	ret = pipe_poll_process(priv, 1);
+	if (ret < 0)
+		return ret;
+
+	/* let copy_write do the actual copying */
+	return copy_write(&priv->stdin, buf, len);
+}
+
+static int pipe_image(struct img_type *img,
+	void __attribute__ ((__unused__)) *data)
+{
+	struct pipe_priv priv = { .status = -1, };
+	int ret, pollret;
+	int stdinpipe[2], stdoutpipe[2], stderrpipe[2];
+	char *cmd = dict_get_value(&img->properties, "cmd");
+	if (!cmd) {
+		ERROR("Pipe handler needs a command to pipe data into: please set the 'cmd' property");
+		return -EINVAL;
+	}
+
+	if (pipe(stdinpipe) < 0) {
+		ERROR("Could not create process pipes");
+		return -EFAULT;
+	}
+	if (pipe(stdoutpipe) < 0) {
+		ERROR("Could not create process pipes");
+		close(stdinpipe[0]);
+		close(stdinpipe[1]);
+		return -EFAULT;
+	}
+	if (pipe(stderrpipe) < 0) {
+		ERROR("Could not create process pipes");
+		close(stdinpipe[0]);
+		close(stdinpipe[1]);
+		close(stdoutpipe[0]);
+		close(stdoutpipe[1]);
+		return -EFAULT;
+	}
+
+	priv.pid = fork();
+	if (priv.pid == 0) {
+		/* child process: cleanup fds and exec */
+		if (dup2(stdinpipe[0], STDIN_FILENO) < 0)
+			exit(1);
+		if (dup2(stdoutpipe[1], STDOUT_FILENO) < 0)
+			exit(1);
+		if (dup2(stderrpipe[1], STDERR_FILENO) < 0)
+			exit(1);
+		close(stdinpipe[0]);
+		close(stdinpipe[1]);
+		close(stdoutpipe[0]);
+		close(stdoutpipe[1]);
+		close(stderrpipe[0]);
+		close(stderrpipe[1]);
+
+		ret = execl("/bin/sh", "sh", "-c", cmd, NULL);
+		ERROR("Cannot execute pipe handler: %s", strerror(errno));
+		exit(1);
+	}
+
+	/* parent process */
+	close(stdinpipe[0]);
+	close(stdoutpipe[1]);
+	close(stderrpipe[1]);
+	priv.stdin = stdinpipe[1];
+	priv.stdout = stdoutpipe[0];
+	priv.stderr = stderrpipe[0];
+
+	/* pipe data to process. Ignoring sigpipe lets the handler error
+	 * properly when writing to a broken pipe instead of exiting */
+	signal(SIGPIPE, SIG_IGN);
+	ret = copyimage(&priv, img, pipe_copy_callback);
+	if (ret < 0) {
+		ERROR("Error copying data to pipe");
+	}
+
+	/* close stdin and keep reading process stdout/stderr until it exits
+	 * (skip if already exited) */
+	close(priv.stdin);
+	if (priv.status == -1) {
+		pollret = pipe_poll_process(&priv, 0);
+		/* keep original error if we had any */
+		if (!ret)
+			ret = pollret;
+	}
+	close(priv.stdout);
+	close(priv.stderr);
+
+	/* empty trailing buffers */
+	if (priv.stdout_index)
+		TRACE("%s", priv.stdout_buf);
+	if (priv.stderr_index)
+		ERROR("%s", priv.stderr_buf);
+
+	TRACE("finished piping image");
+	return ret;
+}
+
+__attribute__((constructor))
+static void pipe_handler(void)
+{
+	register_handler("pipe", pipe_image,
+			 IMAGE_HANDLER | FILE_HANDLER, NULL);
+}