[PATCH 0/3] add pipe handler

[Dominique Martinet]

I actually found some time to work on it today, comments are welcome.
I tried to follow the coding style as possible but I'm sure I missed
some.


- I don't like duplicating code so a good chunk of run_system_cmd()
has been moved to utils read_lines_notify() helper.
There were a couple of corner cases that didn't seem to work so I've
taken a moment to fix these along the way, and retested run_system_cmd
with a shell script.

It might make sense to store the script I used in the repo so if someone
else ever works on it again they can easily have the corner cases I
tried, is there a place for this?
Ideally a real test in the test directory would be best but checking
trace output isn't very practical to do programatically...


- Wrote a general pipe handler as we discussed. I'm happy to change
anything you deem fit.

Wrote handlers.rst and added the handler to all_handlers_defconfig.

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.

In my case, I had already found out that podman is quite fragile (if
e.g. power cuts during an image update even untouched update could
become inaccessible due to broken metadata), so the plan was to create a
new snapshot and write there -- the fail-cmd would be used to destroy
that new snapshot and make sure new data isn't used.
A real attacker could probably find ways to make `podman load` write
outside of its configured directory, but I'll pretend that's a podman
bug and close eyes saying the rest of the filesystem is read-only...
(I'll see if I can make the actual podman load command run as another
user)


Thanks!

Dominique Martinet (3):
utils: add read_lines_notify helper
handlers: add pipe handler
handlers: pipe_handlers: add fail-cmd

configs/all_handlers_defconfig | 1 +
core/pctl.c | 90 +++++--------
core/util.c | 51 +++++++
doc/source/handlers.rst | 29 ++++
handlers/Config.in | 7 +
handlers/Makefile | 1 +
handlers/pipe_handler.c | 236 +++++++++++++++++++++++++++++++++
include/util.h | 2 +
8 files changed, 357 insertions(+), 60 deletions(-)
create mode 100644 handlers/pipe_handler.c

--
2.30.1

[Dominique Martinet]

Move the 'read lines and print to trace/error' logic out of
run_system_cmd into its own helper.

Also fix a few problems, that could be illustrated with the following
script:
---
echo This line is over 256 in length: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Pellentesque posuere sapien quis tempus rutrum. Suspendisse eu mauris tincidunt, aliquam nisl id, laoreet nunc. Curabitur sodales ex at lacus iaculis elementum. Nulla varius tellus at orci dictum et.

sleep 1

echo -n This line takes time to come...
sleep 1
echo Here it is.

sleep 1

echo -n Trailing data
---

problems:
- long line was only printing an arbitrary later part, now print a full
buffer if we get one
- lines weren't continuated over multiple read calls because the
variable was reinitialized when re-entering the read loop
- trailing data was never printed

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---
core/pctl.c | 90 +++++++++++++++++---------------------------------
core/util.c | 51 ++++++++++++++++++++++++++++
include/util.h | 2 ++
3 files changed, 83 insertions(+), 60 deletions(-)

diff --git a/core/pctl.c b/core/pctl.c
index 481f07773e11..ec21bbd9c8f4 100644
--- a/core/pctl.c
+++ b/core/pctl.c
@@ -264,6 +264,9 @@ int run_system_cmd(const char *cmd)
} else {
int fds[2];
pid_t w;
+ char buf[2][SWUPDATE_GENERAL_STRING_SIZE];
+ int cindex[2] = {0, 0}; /* this is the first free char inside buffer */
+ LOGLEVEL levels[2] = { TRACELEVEL, ERRORLEVEL };

close(stdoutpipe[PIPE_WRITE]);
close(stderrpipe[PIPE_WRITE]);
@@ -271,6 +274,14 @@ int run_system_cmd(const char *cmd)
fds[0] = stdoutpipe[PIPE_READ];
fds[1] = stderrpipe[PIPE_READ];

+ /*
+ * Use buffers (for stdout and stdin) to collect data from
+ * the cmd. Data can contain multiple lines or just a part
+ * of a line and must be parsed
+ */
+ memset(buf[0], 0, SWUPDATE_GENERAL_STRING_SIZE);
+ memset(buf[1], 0, SWUPDATE_GENERAL_STRING_SIZE);
+
/*
* Now waits until the child process exits and checks
* for the output. Forward data from stdout as TRACE
@@ -281,9 +292,6 @@ int run_system_cmd(const char *cmd)
struct timeval tv;
fd_set readfds;
int n, i;
- char buf[2][SWUPDATE_GENERAL_STRING_SIZE];
- char *pbuf;
- int cindex[2] = {0, 0}; /* this is the first free char inside buffer */

w = waitpid(process_id, &wstatus, WNOHANG | WUNTRACED | WCONTINUED);
if (w == -1) {
@@ -293,14 +301,6 @@ int run_system_cmd(const char *cmd)
return -EFAULT;
}

- /*
- * Use buffers (for stdout and stdin) to collect data from
- * the cmd. Data can contain multiple lines or just a part
- * of a line and must be parsed
- */
- memset(buf[0], 0, SWUPDATE_GENERAL_STRING_SIZE);
- memset(buf[1], 0, SWUPDATE_GENERAL_STRING_SIZE);
-
tv.tv_sec = 1;
tv.tv_usec = 0;

@@ -316,65 +316,35 @@ int run_system_cmd(const char *cmd)
break;

for (i = 0; i < 2 ; i++) {
- pbuf = buf[i];
- int c = cindex[i];
+ char *pbuf = buf[i];
+ int *c = &cindex[i];
+ LOGLEVEL level = levels[i];

if (FD_ISSET(fds[i], &readfds)) {
- int last;
-
- n = read(fds[i], &pbuf[c], SWUPDATE_GENERAL_STRING_SIZE - c);
+ n = read_lines_notify(fds[i], pbuf, SWUPDATE_GENERAL_STRING_SIZE,
+ c, level);
if (n < 0)
continue;
- n += c; /* add previous data, if any */
n1 += n;
- if (n > 0) {
-
- /* check if just a part of a line was sent. In that
- * case, search for the last line and then copy the rest
- * to the begin of the buffer for next read
- */
- last = n - 1;
- while (last > 0 && pbuf[last] != '\0' && pbuf[last] != '\n')
- last--;
- pbuf[last] = '\0';
- /*
- * compute the truncate line that should be
- * parsed next time when the rest is received
- */
- int left = n - 1 - last;
- char **lines = string_split(pbuf, '\n');
- int nlines = count_string_array((const char **)lines);
-
- if (last < SWUPDATE_GENERAL_STRING_SIZE - 1) {
- last++;
- memcpy(pbuf, &pbuf[last], left);
- if (last + left < SWUPDATE_GENERAL_STRING_SIZE) {
- memset(&pbuf[left], 0, SWUPDATE_GENERAL_STRING_SIZE - left);
- }
- cindex[i] = left;
- } else { /* no need to copy, reuse all buffer */
- memset(pbuf, 0, SWUPDATE_GENERAL_STRING_SIZE);
- cindex[i] = 0;
- }
-
- for (unsigned int index = 0; index < nlines; index++) {
- switch (i) {
- case 0:
- TRACE("%s", lines[index]);
- break;
- case 1:
- ERROR("%s", lines[index]);
- break;
- }
- }
-
- free_string_array(lines);
- }
}
}
} while (ret > 0 && n1 > 0);
} while (w != process_id);

+ /* print any unfinished line */
+ for (int i = 0; i < 2; i++) {
+ if (cindex[i]) {
+ switch(i) {
+ case 0:
+ TRACE("%s", buf[i]);
+ break;
+ case 1:
+ ERROR("%s", buf[i]);
+ break;
+ }
+ }
+ }
+
close(stdoutpipe[PIPE_READ]);
close(stderrpipe[PIPE_READ]);

diff --git a/core/util.c b/core/util.c
index 2025276445dd..0ee536f4b8a7 100644
--- a/core/util.c
+++ b/core/util.c
@@ -842,3 +842,54 @@ size_t snescape(char *dst, size_t n, const char *src)

return len;
}
+
+int read_lines_notify(int fd, char *buf, int buf_size, int *buf_offset,
+ LOGLEVEL level)
+{
+ int n;
+ int offset = *buf_offset;
+ int print_last = 0;
+
+ n = read(fd, &buf[offset], buf_size - offset - 1);
+ if (n <= 0)
+ return -errno;
+
+ n += offset;
+ buf[n] = '\0';
+
+ /*
+ * Check if last line was finished, or if we should copy
+ * it back for next call.
+ * If the buffer is full and there is only one line it
+ * will be printed anyway
+ */
+ if (buf[n-1] == '\n') {
+ print_last = 1;
+ }
+
+ char **lines = string_split(buf, '\n');
+ int nlines = count_string_array((const char **)lines);
+ if (n >= buf_size - 1 && nlines == 1)
+ print_last = 1;
+
+ /* copy leftover data for next call */
+ if (!print_last) {
+ int left = snprintf(buf, buf_size, "%s", lines[nlines-1]);
+ memset(&buf[left], 0, SWUPDATE_GENERAL_STRING_SIZE - left);
+ *buf_offset = left;
+ nlines--;
+ n -= left;
+ } else { /* no need to copy, reuse all buffer */
+ memset(buf, 0, n);
+ *buf_offset = 0;
+ }
+
+ for (unsigned int index = 0; index < nlines; index++) {
+ RECOVERY_STATUS status = level == ERRORLEVEL ? FAILURE : RUN;
+ swupdate_notify(status, "%s", level, lines[index]);
+ }
+
+ free_string_array(lines);
+
+ return n;
+}
diff --git a/include/util.h b/include/util.h
index ad2e90cabd12..76448504f303 100644
--- a/include/util.h
+++ b/include/util.h
@@ -216,6 +216,8 @@ int check_hw_compatibility(struct swupdate_cfg *cfg);
int count_elem_list(struct imglist *list);
unsigned int count_string_array(const char **nodes);
void free_string_array(char **nodes);
+int read_lines_notify(int fd, char *buf, int buf_size, int *buf_offset,
+ LOGLEVEL level);

/* Decryption key functions */
int load_decryption_key(char *fname);
--
2.30.1

[Dominique Martinet]

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...@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...@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);
+}
--
2.30.1

[Dominique Martinet]

Add a mechanism to run yet another command if the copy
failed.
This allows cleanup on error or if the checksum was invalid

Signed-off-by: Dominique Martinet <dominique...@atmark-techno.com>
---

doc/source/handlers.rst | 4 ++++
handlers/pipe_handler.c | 9 +++++++++
2 files changed, 13 insertions(+)

diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
index 764e0d0d09a2..3f5e6e229217 100644
--- a/doc/source/handlers.rst
+++ b/doc/source/handlers.rst
@@ -868,6 +868,10 @@ 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.

+If the "fail-cmd" property has been set, that command will be run if any error
+has been encountered (checksum error, error downloading or writing the file,
+or command returning a non-zero status code)
+

::

diff --git a/handlers/pipe_handler.c b/handlers/pipe_handler.c
index 5c6365614b5c..6a945c94f943 100644
--- a/handlers/pipe_handler.c
+++ b/handlers/pipe_handler.c
@@ -12,6 +12,7 @@
#include "swupdate.h"
#include "handler.h"
#include "util.h"
+#include "pctl.h"

struct pipe_priv {
pid_t pid;
@@ -215,6 +216,14 @@ static int pipe_image(struct img_type *img,
if (priv.stderr_index)
ERROR("%s", priv.stderr_buf);

+ /* run fail command if errored and set */
+ if (ret) {
+ cmd = dict_get_value(&img->properties, "fail-cmd");
+ if (cmd) {
+ run_system_cmd(cmd);
+ }

+ }
+
TRACE("finished piping image");

return ret;
}
--
2.30.1

[Dominique Martinet]

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.

--
Dominique

[Stefano Babic]

Hi Dominique,

On 05.03.21 06:39, Dominique Martinet wrote:
> I actually found some time to work on it today, comments are welcome.
> I tried to follow the coding style as possible but I'm sure I missed
> some.
>
>
> - I don't like duplicating code

Me too - duplication must be avoided

> so a good chunk of run_system_cmd()
> has been moved to utils read_lines_notify() helper.
> There were a couple of corner cases that didn't seem to work so I've
> taken a moment to fix these along the way, and retested run_system_cmd
> with a shell script.
>
> It might make sense to store the script I used in the repo so if someone
> else ever works on it again they can easily have the corner cases I
> tried, is there a place for this?
> Ideally a real test in the test directory would be best but checking
> trace output isn't very practical to do programatically...

There are two option in the repo: if a test can be built and run, it
goes into the test directory. Else it can be stored into the "examples"
directory, this is quite open and contain files that are not part of the
build at all.

>
>
> - Wrote a general pipe handler as we discussed. I'm happy to change
> anything you deem fit.
>
> Wrote handlers.rst and added the handler to all_handlers_defconfig.
>
> 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.

>
> In my case, I had already found out that podman is quite fragile (if
> e.g. power cuts during an image update even untouched update could
> become inaccessible due to broken metadata), so the plan was to create a
> new snapshot and write there -- the fail-cmd would be used to destroy
> that new snapshot and make sure new data isn't used.

It will be nice to document this, too.

> A real attacker could probably find ways to make `podman load` write
> outside of its configured directory, but I'll pretend that's a podman
> bug and close eyes saying the rest of the filesystem is read-only...
> (I'll see if I can make the actual podman load command run as another
> user)

I have a suggestion here.

Best regards,
Stefano Babic

[Stefano Babic]

Hi Dominique,

On 05.03.21 06:39, Dominique Martinet wrote:

> Move the 'read lines and print to trace/error' logic out of
> run_system_cmd into its own helper.

This is a good idea - run_system_cmd() is a too long function and then
difficult to be read.

>
> Also fix a few problems, that could be illustrated with the following
> script:
> ---
> echo This line is over 256 in length: Lorem ipsum dolor sit amet, consectetur adipiscing elit. Pellentesque posuere sapien quis tempus rutrum. Suspendisse eu mauris tincidunt, aliquam nisl id, laoreet nunc. Curabitur sodales ex at lacus iaculis elementum. Nulla varius tellus at orci dictum et.
>

Known, I simply considered unsupported if output is more as 256 bytes.
Thanks for fixing this.

This is not clear to me - what I did in current pctl code, is to scan
the buffer from last char to find a terminating linefeed. You just check
the last char - how can we be sure that the read is synchronized, and
the buffer does not contain the last part of a line and the first part
of the next one ?

I mean : buf could be
<c0><c1>....<cn>\n<d0><d1>...<di>

> +
> + char **lines = string_split(buf, '\n');
> + int nlines = count_string_array((const char **)lines);
> + if (n >= buf_size - 1 && nlines == 1)
> + print_last = 1;
> +
> + /* copy leftover data for next call */
> + if (!print_last) {
> + int left = snprintf(buf, buf_size, "%s", lines[nlines-1]);
> + memset(&buf[left], 0, SWUPDATE_GENERAL_STRING_SIZE - left);

I guess zeroing the read part of the buffer is redundant, is not it ?
The marker is buf_offset.

> + *buf_offset = left;
> + nlines--;
> + n -= left;
> + } else { /* no need to copy, reuse all buffer */
> + memset(buf, 0, n);

Ditto.

> + *buf_offset = 0;
> + }
> +
> + for (unsigned int index = 0; index < nlines; index++) {
> + RECOVERY_STATUS status = level == ERRORLEVEL ? FAILURE : RUN;
> + swupdate_notify(status, "%s", level, lines[index]);
> + }
> +
> + free_string_array(lines);
> +
> + return n;
> +}
> diff --git a/include/util.h b/include/util.h
> index ad2e90cabd12..76448504f303 100644
> --- a/include/util.h
> +++ b/include/util.h
> @@ -216,6 +216,8 @@ int check_hw_compatibility(struct swupdate_cfg *cfg);
> int count_elem_list(struct imglist *list);
> unsigned int count_string_array(const char **nodes);
> void free_string_array(char **nodes);
> +int read_lines_notify(int fd, char *buf, int buf_size, int *buf_offset,
> + LOGLEVEL level);
>
> /* Decryption key functions */
> int load_decryption_key(char *fname);
>

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Dominique Martinet]

Stefano Babic wrote on Sat, Mar 06, 2021 at 10:32:27AM +0100:
> > + /*
> > + * Check if last line was finished, or if we should copy
> > + * it back for next call.
> > + * If the buffer is full and there is only one line it
> > + * will be printed anyway
> > + */
> > + if (buf[n-1] == '\n') {
> > + print_last = 1;
> > + }
>
> This is not clear to me - what I did in current pctl code, is to scan the
> buffer from last char to find a terminating linefeed. You just check the
> last char - how can we be sure that the read is synchronized, and the buffer
> does not contain the last part of a line and the first part of the next one
> ?
>
> I mean : buf could be
> <c0><c1>....<cn>\n<d0><d1>...<di>

That is precisely what the above check (is the last char a line return)
brings.

The key here is that string_split uses strtok, which will always return
the last component of the buffer wether there is a trailing new line or
not -- in its split state, there is no way of making the difference
between these two buffers:
"one line\ntwo lines\n"
"one line\ntwo lines" -- that one is not terminated

By checking beforehand if the last character is a line feed the code
decides later if the last element of the split should be print or copied
back to the start of the buffer.

As for going back to look for earlier new line characters if the last
one wasn't, this can be decided based on the string_split output as done
just below:

> > +
> > + char **lines = string_split(buf, '\n');
> > + int nlines = count_string_array((const char **)lines);
> > + if (n >= buf_size - 1 && nlines == 1)
> > + print_last = 1;

Here if nlines == 1 we know there was no \n before the last character,
so if the last character wasn't a \n already then there is none at all
and we're not printing unless the buffer is full.


I'm not sure how to make this clearer, would inverting the "print_last"
bool to "keep_last" or "copy_last" help hint that we're carrying on
leftover data to the start of the buffer?

> > +
> > + /* copy leftover data for next call */
> > + if (!print_last) {
> > + int left = snprintf(buf, buf_size, "%s", lines[nlines-1]);
> > + memset(&buf[left], 0, SWUPDATE_GENERAL_STRING_SIZE - left);
>
> I guess zeroing the read part of the buffer is redundant, is not it ? The
> marker is buf_offset.

Yes, these are not needed in practice, as the code makes sure there is a
trailing \0 after the read data and snprintf preserves it -- I just kept
these are the orignal code was full of memsets, but the new code is now
far enough that I don't mind removing these.

Note that neither versions would support nul bytes in stdout/stderr
(data in a single read call after a nul byte would silently be dropped),
but since it goes to logging functions I don't think there would be any
reason of handling binary data.


I'll send a v2 without memsets after the line continuation check has
been cleared off
--
Dominique

[Dominique Martinet]

Hi Stefano, thanks for the review.

Stefano Babic wrote on Sat, Mar 06, 2021 at 10:01:33AM +0100:
> > It might make sense to store the script I used in the repo so if someone
> > else ever works on it again they can easily have the corner cases I
> > tried, is there a place for this?
> > Ideally a real test in the test directory would be best but checking
> > trace output isn't very practical to do programatically...
>
> There are two option in the repo: if a test can be built and run, it goes
> into the test directory. Else it can be stored into the "examples"
> directory, this is quite open and contain files that are not part of the
> build at all.

I feel that examples is a bit different, it's probably best to keep it
clear for new users discovering swupdate or people looking for actual
update patterns?

> > - Wrote a general pipe handler as we discussed. I'm happy to change
> > anything you deem fit.
> >
> > Wrote handlers.rst and added the handler to all_handlers_defconfig.
> >
> > 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.

> > In my case, I had already found out that podman is quite fragile (if
> > e.g. power cuts during an image update even untouched update could
> > become inaccessible due to broken metadata), so the plan was to create a
> > new snapshot and write there -- the fail-cmd would be used to destroy
> > that new snapshot and make sure new data isn't used.
>
> It will be nice to document this, too.

I'm not sure if I should document podman-specific behaviour?
I've hinted in-place data modification is not advised when streaming,
could make that more general adding a word about tools that might not be
robust to power loss, but I don't feel comfortable singling out podman
there (unless maybe saying "as of <today's date>" as they are supposedly
working on it a bit)

> > A real attacker could probably find ways to make `podman load` write
> > outside of its configured directory, but I'll pretend that's a podman
> > bug and close eyes saying the rest of the filesystem is read-only...
> > (I'll see if I can make the actual podman load command run as another
> > user)
>
> I have a suggestion here.

Thanks,
--
Dominique

[Stefano Babic]

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.

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.

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.

[Dominique Martinet]

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...@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

--
Dominique

[Stefano Babic]

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...@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.

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
>

[Dominique Martinet]

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,
--
Dominique

[Dominique Martinet]

Ok so let me recap here to keep things regrouped for easier discussion:

Dominique Martinet wrote on Fri, Mar 05, 2021 at 02:39:32PM +0900:
> utils: add read_lines_notify helper

I will resend this patch with the suggested modifications (removing
useless memset, clarifying comment for full line check)

That is useful on its own.

> handlers: add pipe handler
> handlers: pipe_handlers: add fail-cmd

These two as explained in the other thread are no longer useful to me,
but I would still like to add a handler of sort.

Basically, the clumsy way of not setting installed-directly and running
through a shellscript at the end would work, but would require
extracting all archives at once in the prep phase and cause updates that
could fit one step at a time to fail, so having some partial support for
installed-directly would be greatly appreciated.

The main difference with the current iteration of the pipe handler for
podman load in practice would be that swupdate does the write of a
temporary file and podman can use it as is (this time I'm sure of it!
this works as long as the file is not compressed), which also solves my
problem of checksum validation happening after the podman command
started processing the data.


As said in the other thread I don't think making an option for the pipe
handler to do that would make much sense (would it?), so I'm thinking of
having just a simple run_system_cmd() helper:
- copyfile() to temporary location (could be skipped if pre-extracted?)
- build command based on cmd property + temporary filename
- run_system_cmd on that.
- remove temporary file

Alternatively it would be interesting to just make the shellscript
handler run after other files/images (parser-wise would be similar to a
list of "ref"), but that is quite more ambitious in the amount of
modification that brings to the core, so adding a separate handler
sounds more reasonable.


Thanks,
--
Dominique

[Dominique Martinet]

Hi,

Dominique Martinet wrote on Wed, Mar 17, 2021 at 09:59:03AM +0900:
> [...] I would still like to add a handler of sort.

>
> Basically, the clumsy way of not setting installed-directly and running
> through a shellscript at the end would work, but would require
> extracting all archives at once in the prep phase and cause updates that
> could fit one step at a time to fail, so having some partial support for
> installed-directly would be greatly appreciated.
>
> The main difference with the current iteration of the pipe handler for
> podman load in practice would be that swupdate does the write of a
> temporary file and podman can use it as is (this time I'm sure of it!
> this works as long as the file is not compressed), which also solves my
> problem of checksum validation happening after the podman command
> started processing the data.
>
>
> As said in the other thread I don't think making an option for the pipe
> handler to do that would make much sense (would it?), so I'm thinking of
> having just a simple run_system_cmd() helper:
> - copyfile() to temporary location (could be skipped if pre-extracted?)
> - build command based on cmd property + temporary filename
> - run_system_cmd on that.
> - remove temporary file

I never got any feedback on this, would that be considered acceptable or
should I look for an alternative way of doing things?


Basically without such a handler I don't see a way of having swupdate
"feed" scripts images one at a time, which would still be preferable
over extracting everything then loading images.

Alternatively I could just have a script download/look for data in a
fixed location when they want it, but swupdate provides checksuming
(based on a signed file) and encryption for free so I'd rather use it if
possible.

(thinking about it, such a script could also handle piped data without
intermediate copies by making that file a fifo and having copyfile write
to it like the archive handler does; I guess that could be an option for
later if someone is interested...)


Thanks,
--
Dominique