Hi, apart from hotfixes pushed into 2.5.2 there is bunch non critical refactoring work on my table. It is not going to fix any critical issue, but I consider them to be more complete/proper/etc fixes to what was already released or something less serious so it need not go into branch right away.
I'll keep posting the patches into this thread and if do not hear back, I'll push into master after some delay. The code touches critical pieces of our code and and by its nature it's not as narrowly targetted as hotfixes - so the probability something breaks is higher, especially in windows/mac where I can't test/run on my own. Any feedback welcome. The first batch of patches tries to sanitize runCommand() calls. Pavel
>From 877f65aa87401a39d1aa851c47d61120a8ae9ecf Mon Sep 17 00:00:00 2001 From: Pavel Sanda <[email protected]> Date: Wed, 27 May 2026 13:01:14 +0200 Subject: [PATCH 01/11] Implement startscript with cleaner argument handling. New startscriptArgv added, the external calls via runCommand() with user-crafted input would be better to route via this gate and avoid parsecmd/splitCommand parsers altogether. Completion handling shared with original startscript. The argv form is wait-only and does not support TEXINPUTS env prefix or shell-style redirects (no current caller needs them). If a future caller does, extend the API rather than reintroducing the string round-trip. Tier 01 DiD / argv 01: argv-form API; re-closes 00a/00b structurally, removes popen runCommand Assisted-by: Claude Opus 4.7 --- src/support/Systemcall.cpp | 166 ++++++++++++++++++++++++-------- src/support/Systemcall.h | 40 ++++++++ src/support/SystemcallPrivate.h | 17 ++++ 3 files changed, 184 insertions(+), 39 deletions(-) diff --git a/src/support/Systemcall.cpp b/src/support/Systemcall.cpp index ab3fd2ed50..531aa5b7f1 100644 --- a/src/support/Systemcall.cpp +++ b/src/support/Systemcall.cpp @@ -253,6 +253,49 @@ void Systemcall::killscript() } +namespace { + +// Shared tail of startscript() and startscriptArgv(): wait for the +// process spawned via d.startProcess() to start, run, and finish, then +// translate state/exit-code into a Systemcall::ReturnValue. Body is +// the verbatim post-startProcess block extracted from startscript(). +int waitForCompletion(SystemcallPrivate & d, QString const & cmd, + bool do_events, int timeout_ms) +{ + if (d.state == SystemcallPrivate::Error + || !d.waitWhile(SystemcallPrivate::Starting, do_events, -1)) { + if (d.state == SystemcallPrivate::Error) { + LYXERR0("Systemcall: '" << cmd << "' did not start!"); + LYXERR0("error " << d.errorMessage()); + return Systemcall::NOSTART; + } else if (d.state == SystemcallPrivate::Killed) { + LYXERR0("Killed: " << cmd); + return Systemcall::KILLED; + } + } + + if (!d.waitWhile(SystemcallPrivate::Running, do_events, timeout_ms)) { + if (d.state == SystemcallPrivate::Killed) { + LYXERR0("Killed: " << cmd); + return Systemcall::KILLED; + } + LYXERR0("Systemcall: '" << cmd << "' did not finish!"); + LYXERR0("error " << d.errorMessage()); + LYXERR0("status " << d.exitStatusMessage()); + return Systemcall::TIMEOUT; + } + + int const exit_code = d.exitCode(); + if (exit_code) { + LYXERR0("Systemcall: '" << cmd << "' finished with exit code " << exit_code); + } + + return exit_code; +} + +} // namespace + + int Systemcall::startscript(Starttype how, string const & what, string const & path, string const & lpath, bool process_events) @@ -276,36 +319,49 @@ int Systemcall::startscript(Starttype how, string const & what, if (how == DontWait && d.state == SystemcallPrivate::Running) return OK; - if (d.state == SystemcallPrivate::Error - || !d.waitWhile(SystemcallPrivate::Starting, do_events, -1)) { - if (d.state == SystemcallPrivate::Error) { - LYXERR0("Systemcall: '" << cmd << "' did not start!"); - LYXERR0("error " << d.errorMessage()); - return NOSTART; - } else if (d.state == SystemcallPrivate::Killed) { - LYXERR0("Killed: " << cmd); - return KILLED; - } - } + return waitForCompletion(d, cmd, do_events, os::timeout_ms()); +} - if (!d.waitWhile(SystemcallPrivate::Running, do_events, - os::timeout_ms())) { - if (d.state == SystemcallPrivate::Killed) { - LYXERR0("Killed: " << cmd); - return KILLED; - } - LYXERR0("Systemcall: '" << cmd << "' did not finish!"); - LYXERR0("error " << d.errorMessage()); - LYXERR0("status " << d.exitStatusMessage()); - return TIMEOUT; - } - int const exit_code = d.exitCode(); - if (exit_code) { - LYXERR0("Systemcall: '" << cmd << "' finished with exit code " << exit_code); - } +int Systemcall::startscriptArgv(string const & program, + vector<string> const & args, + string * stdout_capture) +{ + // Build the QStringList that will be handed directly to QProcess. + // Each element becomes one argv slot in the child; nothing here + // goes through commandPrep / parsecmd / QProcess::splitCommand, + // so crafted bytes in \p args cannot be re-split into a different + // argv than the caller wrote. That is the security property of + // this entry point. + QString const qprogram = QString::fromLocal8Bit(program.c_str()); + QStringList qargs; + qargs.reserve(int(args.size())); + for (string const & a : args) + qargs << QString::fromLocal8Bit(a.c_str()); + + // Display string for log / cancel-dialog. Never executed. + QString display = qprogram; + for (QString const & a : qargs) + display += QLatin1Char(' ') + a; - return exit_code; + if (verbose) + lyxerr << "\nRunning: " << fromqstr(display) << endl; + else + LYXERR(Debug::INFO, "Running: " << fromqstr(display)); + + string const empty; + SystemcallPrivate d(empty, empty, empty); + QByteArray captured; + if (stdout_capture) + d.setStdoutCapture(&captured); + + d.startProcess(qprogram, qargs, display); + int const ret = waitForCompletion(d, display, false, os::timeout_ms()); + + if (stdout_capture) + stdout_capture->assign(captured.constData(), captured.size()); + + return ret; } @@ -315,7 +371,8 @@ bool SystemcallPrivate::kill_script = false; SystemcallPrivate::SystemcallPrivate(std::string const & sf, std::string const & of, std::string const & ef) : state(Error), process_(new QProcess), out_index_(0), err_index_(0), - in_file_(sf), out_file_(of), err_file_(ef), process_events_(false) + in_file_(sf), out_file_(of), err_file_(ef), process_events_(false), + capture_stdout_(nullptr) { if (!in_file_.empty()) process_->setStandardInputFile(QString::fromLocal8Bit(in_file_.c_str())); @@ -432,6 +489,31 @@ void SystemcallPrivate::startProcess(QString const & cmd, string const & path, } +void SystemcallPrivate::startProcess(QString const & program, + QStringList const & args, + QString const & display) +{ + // cmd_ holds the display string used by the cancel dialog and log + // messages only. The actual launch goes through QProcess::start + // with (program, args) as separate arguments, bypassing + // QProcess::splitCommand entirely - so nothing between this point + // and the child's argv vector can reshape it. Detached/DontWait + // is not supported for the argv form (the sole caller is + // Systemcall::startscriptArgv, which is wait-only). + cmd_ = display.isEmpty() ? program : display; + if (process_) { + state = SystemcallPrivate::Starting; + process_->start(program, args); + } +} + + +void SystemcallPrivate::setStdoutCapture(QByteArray * dest) +{ + capture_stdout_ = dest; +} + + bool SystemcallPrivate::waitAndCheck() { Sleep::millisec(100); @@ -551,17 +633,22 @@ SystemcallPrivate::~SystemcallPrivate() void SystemcallPrivate::stdOut() { - if (process_) { - char c; - process_->setReadChannel(QProcess::StandardOutput); - while (process_->getChar(&c)) { - out_data_[out_index_++] = c; - if (c == '\n' || out_index_ + 1 == buffer_size_) { - out_data_[out_index_] = '\0'; - out_index_ = 0; - ProgressInterface::instance()->appendMessage(QString::fromLocal8Bit(out_data_)); - cout << out_data_; - } + if (!process_) + return; + if (capture_stdout_) { + // argv-form callers want raw stdout in a buffer, not echoed. + capture_stdout_->append(process_->readAllStandardOutput()); + return; + } + char c; + process_->setReadChannel(QProcess::StandardOutput); + while (process_->getChar(&c)) { + out_data_[out_index_++] = c; + if (c == '\n' || out_index_ + 1 == buffer_size_) { + out_data_[out_index_] = '\0'; + out_index_ = 0; + ProgressInterface::instance()->appendMessage(QString::fromLocal8Bit(out_data_)); + cout << out_data_; } } } diff --git a/src/support/Systemcall.h b/src/support/Systemcall.h index d73a928097..e125873695 100644 --- a/src/support/Systemcall.h +++ b/src/support/Systemcall.h @@ -15,6 +15,9 @@ #ifndef SYSTEMCALL_H #define SYSTEMCALL_H +#include <string> +#include <vector> + namespace lyx { namespace support { @@ -64,8 +67,38 @@ public: std::string const & path = empty_string(), std::string const & lpath = empty_string(), bool process_events = false); + + /** Argv-form invocation: shell-free, parser-free. + * + * Builds an argv vector from (program, args) and hands it directly + * to QProcess::start(program, QStringList), which calls execvp() + * on Unix and CreateProcess() on Windows. Each element of \p args + * reaches the child as one argv slot, byte-for-byte. + * + * Note: although string-form startscript() does not invoke a shell + * it resplits into different argv, which is weak from the start. + * + * Wait-only (no DontWait/WaitLoop equivalent), no path/lpath + * TEXINPUTS prefix, no shell-style redirects. If a future caller + * needs any of those, extend the API rather than reintroducing the + * string round-trip. + * + * \p stdout_capture if non-null, child stdout is captured here. + */ + int startscriptArgv(std::string const & program, + std::vector<std::string> const & args, + std::string * stdout_capture = nullptr); }; + +/// Split a shell-style command string into space-separated tokens, with +/// double-quoted tokens copied verbatim. Used to migrate string-form +/// invocations to the argv-form of Systemcall::startscriptArgv without +/// going through a shell. NOT a full shell parser: no $VAR / $() / +/// backslash escape handling, no single-quote handling. +std::vector<std::string> splitCommandTokens(std::string const & cmd); + + } // namespace support } // namespace lyx diff --git a/src/support/SystemcallPrivate.h b/src/support/SystemcallPrivate.h index 19771dfc73..42cb922696 100644 --- a/src/support/SystemcallPrivate.h +++ b/src/support/SystemcallPrivate.h @@ -12,8 +12,10 @@ #ifndef SYSTEMCALLPRIVATE_H #define SYSTEMCALLPRIVATE_H +#include <QByteArray> #include <QObject> #include <QProcess> +#include <QStringList> #include <string> @@ -47,6 +49,19 @@ public: void startProcess(QString const & cmd, std::string const & path, std::string const & lpath, bool detach); + /// Argv-form launch: \p program and \p args go directly to + /// QProcess::start(program, args), bypassing QProcess::splitCommand + /// (which the string-form overload above uses to recover argv from + /// the command line). The child sees each element of \p args as + /// one argv slot byte-for-byte. \p display is shown in log messages + /// and the cancel-dialog text but is never executed. Wait-only. + void startProcess(QString const & program, QStringList const & args, + QString const & display); + + /// When non-null, stdOut() routes incoming bytes into *dest + /// instead of echoing to the progress UI / std::cout. + void setStdoutCapture(QByteArray * dest); + int exitCode(); QString errorMessage() const; @@ -92,6 +107,8 @@ private: QString cmd_; bool process_events_; + /// If non-null, stdOut() appends here instead of echoing. + QByteArray * capture_stdout_; void waitAndProcessEvents(); void processEvents(); -- 2.39.5
-- lyx-devel mailing list [email protected] https://lists.lyx.org/mailman/listinfo/lyx-devel
