From c374653c7bfc7450590d6a787bd4f413d15a76af Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 17 Jul 2023 12:01:47 +0200 Subject: [PATCH 1/3] Revert "Merge pull request #52138 from azat/decompressor-inode" This reverts commit 6524031348c3a1148e27ef926baabb35e833a09c, reversing changes made to 9bf114f9a36556d6f227dea0fa91be131ee99710. This was not a good idea, since the underlying problem was that `/proc/self/exe` was pointing to `qemu-$ARCH-static` (because the code uses realpath() over normal interface readlink(), which is not caught by the qemu linux-user). And this means that later, it will try to overwrite incorrect binary and then execute some garbage. Signed-off-by: Azat Khuzhin --- .../decompressor.cpp | 93 +++++++++---------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/utils/self-extracting-executable/decompressor.cpp b/utils/self-extracting-executable/decompressor.cpp index 4a4985120fd..d41b9b1ebe1 100644 --- a/utils/self-extracting-executable/decompressor.cpp +++ b/utils/self-extracting-executable/decompressor.cpp @@ -430,58 +430,55 @@ int main(int/* argc*/, char* argv[]) return 1; } - int lock = -1; - /// Protection from double decompression #if !defined(OS_DARWIN) && !defined(OS_FREEBSD) /// get inode of this executable uint64_t inode = getInode(self); - /// In some cases /proc/self/maps may not contain the inode for the - /// /proc/self/exe, one of such examples are using qemu-*-static, in this - /// case maps will be proxied through the qemu, and it will remove - /// information about itself from it. - if (inode != 0) + if (inode == 0) { - std::stringstream lock_path; // STYLE_CHECK_ALLOW_STD_STRING_STREAM - lock_path << "/tmp/" << name << ".decompression." << inode << ".lock"; - lock = open(lock_path.str().c_str(), O_CREAT | O_RDWR, 0666); - if (lock < 0) + std::cerr << "Unable to obtain inode." << std::endl; + return 1; + } + + std::stringstream lock_path; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + lock_path << "/tmp/" << name << ".decompression." << inode << ".lock"; + int lock = open(lock_path.str().c_str(), O_CREAT | O_RDWR, 0666); + if (lock < 0) + { + perror("lock open"); + return 1; + } + + /// lock file should be closed on exec call + fcntl(lock, F_SETFD, FD_CLOEXEC); + + if (lockf(lock, F_LOCK, 0)) + { + perror("lockf"); + return 1; + } + + /// inconsistency in WSL1 Ubuntu - inode reported in /proc/self/maps is a 64bit to + /// 32bit conversion of input_info.st_ino + if (input_info.st_ino & 0xFFFFFFFF00000000 && !(inode & 0xFFFFFFFF00000000)) + input_info.st_ino &= 0x00000000FFFFFFFF; + + /// if decompression was performed by another process since this copy was started + /// then file referred by path "self" is already pointing to different inode + if (input_info.st_ino != inode) + { + struct stat lock_info; + if (0 != fstat(lock, &lock_info)) { - perror("lock open"); + perror("fstat lock"); return 1; } - /// lock file should be closed on exec call - fcntl(lock, F_SETFD, FD_CLOEXEC); + /// size 1 of lock file indicates that another decompressor has found active executable + if (lock_info.st_size == 1) + execv(self, argv); - if (lockf(lock, F_LOCK, 0)) - { - perror("lockf"); - return 1; - } - - /// inconsistency in WSL1 Ubuntu - inode reported in /proc/self/maps is a 64bit to - /// 32bit conversion of input_info.st_ino - if (input_info.st_ino & 0xFFFFFFFF00000000 && !(inode & 0xFFFFFFFF00000000)) - input_info.st_ino &= 0x00000000FFFFFFFF; - - /// if decompression was performed by another process since this copy was started - /// then file referred by path "self" is already pointing to different inode - if (input_info.st_ino != inode) - { - struct stat lock_info; - if (0 != fstat(lock, &lock_info)) - { - perror("fstat lock"); - return 1; - } - - /// size 1 of lock file indicates that another decompressor has found active executable - if (lock_info.st_size == 1) - execv(self, argv); - - printf("No target executable - decompression only was performed.\n"); - return 0; - } + printf("No target executable - decompression only was performed.\n"); + return 0; } #endif @@ -549,19 +546,21 @@ int main(int/* argc*/, char* argv[]) if (has_exec) { +#if !defined(OS_DARWIN) && !defined(OS_FREEBSD) /// write one byte to the lock in case other copies of compressed are running to indicate that /// execution should be performed - if (lock >= 0) - write(lock, "1", 1); + write(lock, "1", 1); +#endif execv(self, argv); /// This part of code will be reached only if error happened perror("execv"); return 1; } +#if !defined(OS_DARWIN) && !defined(OS_FREEBSD) /// since inodes can be reused - it's a precaution if lock file already exists and have size of 1 - if (lock >= 0) - ftruncate(lock, 0); + ftruncate(lock, 0); +#endif printf("No target executable - decompression only was performed.\n"); } From 16165d9498cbebd3ecd02df87480230fd0ed880e Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 17 Jul 2023 12:02:54 +0200 Subject: [PATCH 2/3] Improve error messages for decompressor Signed-off-by: Azat Khuzhin --- utils/self-extracting-executable/decompressor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/self-extracting-executable/decompressor.cpp b/utils/self-extracting-executable/decompressor.cpp index d41b9b1ebe1..567d9088f13 100644 --- a/utils/self-extracting-executable/decompressor.cpp +++ b/utils/self-extracting-executable/decompressor.cpp @@ -435,7 +435,7 @@ int main(int/* argc*/, char* argv[]) uint64_t inode = getInode(self); if (inode == 0) { - std::cerr << "Unable to obtain inode." << std::endl; + std::cerr << "Unable to obtain inode for exe '" << self << "'." << std::endl; return 1; } From 1fb7605fb4225225d492cf63f7f048d594e89fc3 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Mon, 17 Jul 2023 12:09:11 +0200 Subject: [PATCH 3/3] Fix self extracting binaries under qemu linux-user (qemu-$ARCH-static) The problem was that the decompressor uses realpath(/proc/self/exe) instead of readlink(/proc/self/exe), while realpath() does lots of trickerly [1] which leads to bypassing qemu linux-user override [2] of /proc/self/exe to the executable with with it had been called -- and the reason for this is that the getpid() after unshare returns 1, while reading /proc/self returns the pid that was before unshare (from the chroot) [3]. [1]: https://github.com/bminor/glibc/blob/4290aed05135ae4c0272006442d147f2155e70d7/stdlib/canonicalize.c#L223 [2]: https://github.com/qemu/qemu/blob/ed8ad9728a9c0eec34db9dff61dfa2f1dd625637/linux-user/syscall.c#L8634 [3]: https://gist.github.com/azat/fcbd8b6c26afd505ae5f3387fc15f0e2 But note, that even after this patch qemu without binfmt will not work, due to internally the code calls execv() while qemu does not handle it (see [4]). [4]: https://patchwork.kernel.org/project/qemu-devel/patch/1453091602-21843-1-git-send-email-petrosagg@gmail.com/ Signed-off-by: Azat Khuzhin --- utils/self-extracting-executable/decompressor.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/utils/self-extracting-executable/decompressor.cpp b/utils/self-extracting-executable/decompressor.cpp index 567d9088f13..91f4bea5a5b 100644 --- a/utils/self-extracting-executable/decompressor.cpp +++ b/utils/self-extracting-executable/decompressor.cpp @@ -362,11 +362,12 @@ int decompressFiles(int input_fd, char * path, char * name, bool & have_compress #else - int read_exe_path(char *exe, size_t/* buf_sz*/) + int read_exe_path(char *exe, size_t buf_sz) { - if (realpath("/proc/self/exe", exe) == nullptr) - return 1; - return 0; + ssize_t n = readlink("/proc/self/exe", exe, buf_sz - 1); + if (n > 0) + exe[n] = '\0'; + return n > 0 && n < static_cast(buf_sz); } #endif