From 4e00671dccc4e214ed4ebecfe6892de84c881bff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Marczewski?= Date: Mon, 5 Jul 2021 14:06:45 +0200 Subject: [PATCH] [LibOS] Fix a crash after reusing dentry touched by `mknod` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a crash where we create a named pipe, remove it, and then try to create a normal file with the same name. Unfortunately, fixing the underlying problem (dentry reuse) is more problematic, so this is only a workaround for that specific case. Signed-off-by: Paweł Marczewski --- LibOS/shim/src/fs/shim_namei.c | 22 ++++++++++++++++++++++ LibOS/shim/test/regression/mkfifo.c | 19 ++++++++++++++++++- LibOS/shim/test/regression/test_libos.py | 7 ++++++- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/LibOS/shim/src/fs/shim_namei.c b/LibOS/shim/src/fs/shim_namei.c index 256485e7a9..115cbce092 100644 --- a/LibOS/shim/src/fs/shim_namei.c +++ b/LibOS/shim/src/fs/shim_namei.c @@ -58,6 +58,24 @@ static struct shim_dentry* lookup_dcache_or_create(struct shim_dentry* parent, c return dent; } +/* + * HACK: Reset dentry state, in case this is a dentry for which we changed `fs` earlier. + * + * This is a workaround for a situation in which we create a special node (named pipe or socket) and + * then delete or rename it. Currently that marks the old dentry as `DENTRY_NEGATIVE` and allows it + * to be reused later, despite changed `fs` field. + * + * Ideally, we should always start with a fresh dentry, which would ensure all fields are in a valid + * state. However, that requires fixing the memory leak of `dent->data` first (see `free_dentry()` + * in `shim_dcache.c`). + */ +static void reset_dentry(struct shim_dentry* dent) { + if (dent->fs != dent->mount->fs) { + dent->fs = dent->mount->fs; + dent->data = NULL; + } +} + /*! * \brief Validate a dentry, if necessary * @@ -82,6 +100,8 @@ static int validate_dentry(struct shim_dentry* dent) { if (dent->state & DENTRY_VALID) return 0; + reset_dentry(dent); + /* This is an invalid dentry: either we just created it, or it got left over from a previous * failed lookup. Perform the lookup. */ assert(dent->fs); @@ -559,6 +579,8 @@ int open_namei(struct shim_handle* hdl, struct shim_dentry* start, const char* p if (ret < 0) goto err; + reset_dentry(dent); + /* Create directory or file, depending on O_DIRECTORY. Return -EINVAL if the operation is * not supported for this filesystem. */ if (flags & O_DIRECTORY) { diff --git a/LibOS/shim/test/regression/mkfifo.c b/LibOS/shim/test/regression/mkfifo.c index a98e7aaa84..93cfccd263 100644 --- a/LibOS/shim/test/regression/mkfifo.c +++ b/LibOS/shim/test/regression/mkfifo.c @@ -9,7 +9,7 @@ #include #include -#define FIFO_PATH "fifo123" +#define FIFO_PATH "tmp/fifo" int main(int argc, char** argv) { int fd; @@ -93,6 +93,23 @@ int main(int argc, char** argv) { perror("[parent] unlink error"); return 1; } + + /* Check if we can create a normal file with the same name. */ + fd = open(FIFO_PATH, O_CREAT | O_TRUNC, 0600); + if (fd < 0) { + perror("[parent] open error"); + return 1; + } + if (close(fd) < 0) { + perror("[parent] close error"); + return 1; + } + if (unlink(FIFO_PATH) < 0) { + perror("[parent] unlink error"); + return 1; + } + + printf("[parent] TEST OK\n"); } return 0; diff --git a/LibOS/shim/test/regression/test_libos.py b/LibOS/shim/test/regression/test_libos.py index 9062be63b8..33f68e0a04 100644 --- a/LibOS/shim/test/regression/test_libos.py +++ b/LibOS/shim/test/regression/test_libos.py @@ -874,8 +874,13 @@ def test_092_pipe_ocloexec(self): self.assertIn('TEST OK', stdout) def test_095_mkfifo(self): - stdout, _ = self.run_binary(['mkfifo'], timeout=60) + try: + stdout, _ = self.run_binary(['mkfifo'], timeout=60) + finally: + if os.path.exists('tmp/fifo'): + os.remove('tmp/fifo') self.assertIn('read on FIFO: Hello from write end of FIFO!', stdout) + self.assertIn('[parent] TEST OK', stdout) def test_100_socket_unix(self): stdout, _ = self.run_binary(['unix'])