From 54771e6ddf3ec0fe258405bc32bb311dc10a2527 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Thu, 29 May 2025 21:36:42 +0200 Subject: [PATCH 1/5] Minor fixes to ChildProcess - Don't set FD_CLOEXEC, but close manually before fork: Since we don't use pipe2() to set the flag atomically, we're prone to races with concurrent fork+execs either way. As ChildProcess uses a mutex here anyways, as long as there is no other part in lms that would fork+exec, we're not any more or less safe now, but the code is shorter and we cannot fail the fcntl. - Don't make the write-end of the pipe non-blocking. Usually programs do not expect this, i.e. they either never check the return code of a write to stdout (potential data loss), or if they do, they just bail out on any error, and don't handle EAGAIN. ffmpeg seemed to handle this fine though (or we were just lucky and always read the data faster than ffmpeg could produce it). - Do not close stdin and stderr. Again ffmpeg seems to handle this, at least regarding stdin thanks to -nostdin, but in general if a process tries to write to stderr and fd 2 is not open, it might just bail out. Even worse, the process might have opened some file it wants to work with, and that file got assigned fd 2 (as that was the next free fd) - the program would corrupt whatever file it opened there whenever it tries to write to stderr. Try to open /dev/null instead for stdin and stderr, and if that fails, keep whatever lms inherited open, which should be safer than relying on the child process to handle this case properly. - exec() does not return on success, no need to check return code, any return from it is a failure. - Wrong error message in error path when assigning fd to boost stream. - F_SETPIPE_SZ requires and int, not size_t. This worked on little endian systems since the value passed was < 2^32, but on big endian systems you'd effectively pass "0" if using a 64bit type. - Address a few clang-tidy complaints (constness) --- src/libs/core/impl/ChildProcess.cpp | 44 ++++++++++++++--------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/libs/core/impl/ChildProcess.cpp b/src/libs/core/impl/ChildProcess.cpp index 2dc57207..157b7709 100644 --- a/src/libs/core/impl/ChildProcess.cpp +++ b/src/libs/core/impl/ChildProcess.cpp @@ -63,32 +63,23 @@ namespace lms::core { // make sure only one thread is executing this part of code static std::mutex mutex; - std::unique_lock lock{ mutex }; + const std::lock_guard lock{ mutex }; int pipefd[2]; // Use 'pipe' instead of 'pipe2', more portable - if (pipe(pipefd) < 0) + if (pipe(pipefd) == -1) throw SystemException{ std::error_code{ errno, std::generic_category() }, "pipe failed!" }; - // Manually set the O_NONBLOCK and O_CLOEXEC flags for both ends of the pipe + // Only set O_NONBLOCK on read end - usually programs don't expect stdout to be non-blocking if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1) throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set O_NONBLOCK!" }; - if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1) - throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set O_NONBLOCK!" }; - - if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1) - throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set FD_CLOEXEC!" }; - - if (fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1) - throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set FD_CLOEXEC!" }; - #if defined(__linux__) && defined(F_SETPIPE_SZ) for (const int fd : { pipefd[0], pipefd[1] }) { - constexpr std::size_t targetPipeSize{ static_cast(65'536) * 4 }; - std::size_t currentPipeSize{ 65'536 }; // common default value + constexpr int targetPipeSize{ 65'536 * 4 }; + int currentPipeSize{ 65'536 }; // common default value #if defined(F_GETPIPE_SZ) const int pipeSizeRes{ fcntl(fd, F_GETPIPE_SZ) }; if (pipeSizeRes == -1) @@ -112,27 +103,36 @@ namespace lms::core } #endif - int res{ fork() }; + const int res{ fork() }; if (res == -1) throw SystemException{ std::error_code{ errno, std::generic_category() }, "fork failed!" }; if (res == 0) // CHILD { - close(pipefd[0]); - close(STDIN_FILENO); - close(STDERR_FILENO); + // Never close stdin/out/err, most programs expect these to exist; + // rather connect them to /dev/null if unwanted + const int nullFd{ open("/dev/null", O_RDWR) }; + // Ignore errors, worst thing is stderr writes to the same fd as lms + if (nullFd != -1) + { + dup2(nullFd, STDIN_FILENO); + dup2(nullFd, STDERR_FILENO); + close(nullFd); + } // Replace stdout with pipe write if (dup2(pipefd[1], STDOUT_FILENO) == -1) exit(-1); + // Close pipe: read end not needed, write end was dup2ed + close(pipefd[0]); + close(pipefd[1]); std::vector execArgs; std::transform(std::cbegin(args), std::cend(args), std::back_inserter(execArgs), [](const std::string& arg) { return arg.c_str(); }); execArgs.push_back(nullptr); - res = execv(path.string().c_str(), (char* const*)&execArgs[0]); - if (res == -1) - exit(-1); + execv(path.string().c_str(), (char* const*)&execArgs[0]); + exit(-1); } else // PARENT { @@ -141,7 +141,7 @@ namespace lms::core boost::system::error_code assignError; _childStdout.assign(pipefd[0], assignError); if (assignError) - throw SystemException{ assignError, "fork failed!" }; + throw SystemException{ assignError, "assigning read end of pipe to asio stream failed!" }; } _childPID = res; } From 1e7fbcad1590d91bef87ca4dfcd013baaa893d6b Mon Sep 17 00:00:00 2001 From: emeric Date: Mon, 2 Jun 2025 21:10:01 +0200 Subject: [PATCH 2/5] Fixed warning --- src/libs/services/transcoding/impl/TranscodingService.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/services/transcoding/impl/TranscodingService.cpp b/src/libs/services/transcoding/impl/TranscodingService.cpp index 1599d3a1..09270a19 100644 --- a/src/libs/services/transcoding/impl/TranscodingService.cpp +++ b/src/libs/services/transcoding/impl/TranscodingService.cpp @@ -38,7 +38,7 @@ namespace lms::transcoding std::size_t doEstimateContentLength(std::size_t bitrate, std::chrono::milliseconds duration) { - const std::size_t estimatedContentLength{ (bitrate / 8 * duration.count()) / 1000 }; + const std::size_t estimatedContentLength{ static_cast((bitrate / 8 * duration.count()) / 1000) }; return estimatedContentLength; } } // namespace From b2471cccfb6511f9ec336df0fad84317e8be2125 Mon Sep 17 00:00:00 2001 From: Simon Rettberg Date: Sat, 24 May 2025 15:49:47 +0200 Subject: [PATCH 3/5] FileResourceHandler: Open file once on init, more error handling Keeping the file open allows better prefetching/caching on both the application layer (ifstream) and OS level (vfs layer) as the access pattern is sequential and predictable. We also save three syscalls (open, seek, close) for every chunk we send to the client (256kb). --- src/libs/core/impl/FileResourceHandler.cpp | 75 ++++++++++------------ src/libs/core/impl/FileResourceHandler.hpp | 4 +- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/src/libs/core/impl/FileResourceHandler.cpp b/src/libs/core/impl/FileResourceHandler.cpp index 7db88efc..561fda0f 100644 --- a/src/libs/core/impl/FileResourceHandler.cpp +++ b/src/libs/core/impl/FileResourceHandler.cpp @@ -19,8 +19,6 @@ #include "FileResourceHandler.hpp" -#include - #include "core/ILogger.hpp" #include "core/MimeTypes.hpp" @@ -32,38 +30,37 @@ namespace lms::core } FileResourceHandler::FileResourceHandler(const std::filesystem::path& path, std::string_view mimeType) - : _path{ path } - , _mimeType{ mimeType } + : _mimeType{ mimeType } + , _ifs{ path, std::ios::in | std::ios::binary } { + if (!_ifs) + LMS_LOG(UTILS, ERROR, "Cannot open file stream for " << path); + else + { + _ifs.seekg(0, std::ios::end); + if (!_ifs.fail()) + _fileSize = static_cast<::uint64_t>(_ifs.tellg()); + LMS_LOG(UTILS, DEBUG, "File " << path << ", fileSize = " << _fileSize); + } } Wt::Http::ResponseContinuation* FileResourceHandler::processRequest(const Wt::Http::Request& request, Wt::Http::Response& response) { - ::uint64_t startByte{ _offset }; - std::ifstream ifs{ _path, std::ios::in | std::ios::binary }; - - if (startByte == 0) + if (_offset == 0) { - if (!ifs) + if (!_ifs) { - LMS_LOG(UTILS, ERROR, "Cannot open file stream for " << _path); response.setStatus(404); return {}; } - ifs.seekg(0, std::ios::end); - const ::uint64_t fileSize{ static_cast<::uint64_t>(ifs.tellg()) }; - ifs.seekg(0, std::ios::beg); - - LMS_LOG(UTILS, DEBUG, "File " << _path << ", fileSize = " << fileSize); - response.addHeader("Accept-Ranges", "bytes"); - const Wt::Http::Request::ByteRangeSpecifier ranges{ request.getRanges(fileSize) }; + const Wt::Http::Request::ByteRangeSpecifier ranges{ request.getRanges(_fileSize) }; if (!ranges.isSatisfiable()) { std::ostringstream contentRange; - contentRange << "bytes */" << fileSize; + contentRange << "bytes */" << _fileSize; response.setStatus(416); // Requested range not satisfiable response.addHeader("Content-Range", contentRange.str()); @@ -76,57 +73,51 @@ namespace lms::core LMS_LOG(UTILS, DEBUG, "Range requested = " << ranges[0].firstByte() << "-" << ranges[0].lastByte()); response.setStatus(206); - startByte = ranges[0].firstByte(); + _offset = ranges[0].firstByte(); _beyondLastByte = ranges[0].lastByte() + 1; std::ostringstream contentRange; - contentRange << "bytes " << startByte << "-" - << _beyondLastByte - 1 << "/" << fileSize; + contentRange << "bytes " << _offset << "-" + << _beyondLastByte - 1 << "/" << _fileSize; response.addHeader("Content-Range", contentRange.str()); - response.setContentLength(_beyondLastByte - startByte); + response.setContentLength(_beyondLastByte - _offset); } else { LMS_LOG(UTILS, DEBUG, "No range requested"); response.setStatus(200); - _beyondLastByte = fileSize; + _beyondLastByte = _fileSize; response.setContentLength(_beyondLastByte); } LMS_LOG(UTILS, DEBUG, "Mimetype set to '" << _mimeType << "'"); response.setMimeType(_mimeType); - } - else if (!ifs) - { - LMS_LOG(UTILS, ERROR, "Cannot reopen file stream for " << _path); - return {}; - } - ifs.seekg(static_cast(startByte)); + _ifs.seekg(static_cast(_offset)); + } // end initial response setup - std::vector buf; - buf.resize(_chunkSize); + ::uint64_t restSize = _beyondLastByte - _offset; + ::uint64_t pieceSize = std::min(restSize, _chunkSize); - ::uint64_t restSize = _beyondLastByte - startByte; - ::uint64_t pieceSize = buf.size() > restSize ? restSize : buf.size(); + std::vector buf(pieceSize); - ifs.read(&buf[0], pieceSize); - const ::uint64_t actualPieceSize{ static_cast<::uint64_t>(ifs.gcount()) }; + _ifs.read(buf.data(), buf.size()); + const ::uint64_t actualPieceSize{ static_cast<::uint64_t>(_ifs.gcount()) }; if (actualPieceSize > 0) { - response.out().write(&buf[0], actualPieceSize); - LMS_LOG(UTILS, DEBUG, "Written " << actualPieceSize << " bytes, range = " << startByte << "-" << startByte + actualPieceSize - 1 << ""); + response.out().write(buf.data(), actualPieceSize); + LMS_LOG(UTILS, DEBUG, "Written " << actualPieceSize << " bytes, range = " << _offset << "-" << _offset + actualPieceSize - 1 << ""); } else - { LMS_LOG(UTILS, DEBUG, "Written 0 byte"); - } - if (ifs.good() && actualPieceSize < restSize) + if (!_ifs.good()) + LMS_LOG(UTILS, WARNING, "Error reading from file!"); + else if (actualPieceSize < restSize) { - _offset = startByte + actualPieceSize; + _offset += actualPieceSize; LMS_LOG(UTILS, DEBUG, "Job not complete! Remaining range: " << _offset << "-" << _beyondLastByte - 1); return response.createContinuation(); diff --git a/src/libs/core/impl/FileResourceHandler.hpp b/src/libs/core/impl/FileResourceHandler.hpp index 7e541038..5c239842 100644 --- a/src/libs/core/impl/FileResourceHandler.hpp +++ b/src/libs/core/impl/FileResourceHandler.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include "core/IResourceHandler.hpp" @@ -38,9 +39,10 @@ namespace lms::core static constexpr std::size_t _chunkSize{ 262'144 }; - std::filesystem::path _path; std::string _mimeType; ::uint64_t _beyondLastByte{}; ::uint64_t _offset{}; + ::uint64_t _fileSize{}; + std::ifstream _ifs; }; } // namespace lms::core \ No newline at end of file From c9e4ec1eb5bcd4bfddc3780723d5ceb0561543f6 Mon Sep 17 00:00:00 2001 From: emeric Date: Mon, 2 Jun 2025 21:38:19 +0200 Subject: [PATCH 4/5] fixed format --- src/libs/core/impl/FileResourceHandler.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/core/impl/FileResourceHandler.hpp b/src/libs/core/impl/FileResourceHandler.hpp index 5c239842..34beecc4 100644 --- a/src/libs/core/impl/FileResourceHandler.hpp +++ b/src/libs/core/impl/FileResourceHandler.hpp @@ -20,9 +20,9 @@ #pragma once #include +#include #include #include -#include #include "core/IResourceHandler.hpp" From 161d7b6c152b935de064233829d38b73439d0e1b Mon Sep 17 00:00:00 2001 From: emeric Date: Fri, 6 Jun 2025 08:45:34 +0200 Subject: [PATCH 5/5] Minor clean --- src/libs/core/impl/ChildProcess.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libs/core/impl/ChildProcess.cpp b/src/libs/core/impl/ChildProcess.cpp index 157b7709..dfcb532f 100644 --- a/src/libs/core/impl/ChildProcess.cpp +++ b/src/libs/core/impl/ChildProcess.cpp @@ -19,18 +19,17 @@ #include "ChildProcess.hpp" -#include -#include #include #include #include #include -#include #include #include -#include +#include +#include #include +#include #include #include @@ -63,7 +62,7 @@ namespace lms::core { // make sure only one thread is executing this part of code static std::mutex mutex; - const std::lock_guard lock{ mutex }; + const std::scoped_lock lock{ mutex }; int pipefd[2];