On Thu, Sep 17, 2026 at 10:02:33PM +0200, Enrico Forestieri wrote:
> On Thu, Sep 17, 2026 at 03:02:05AM +0200, Pavel Sanda wrote:
> > On Sun, Sep 13, 2026 at 07:05:33PM +0200, Pavel Sanda wrote:
> > > Ok, with 03 class finished (invocation on load) I'll continue with
> > > misc lefotvers (04 class).
> > >
> > > First - guard better esoteric biblio paperviewer from context menu.
> >
> > Last but one. Kicking runCommand() from CAS handling.
> >
> > To be clear, the current form is safe and if I was not pushing shell based
> > runCommand() out of source code, it could stay as it is.
> >
> > The rewrite is not mechanical and it would be great if anyone on Windows
> > could test that e.g. maxima still delivers results in mathed.
> > On linux it works (tested octave and maxima).
>
> Hi Pavel, on native Windows this patch works only with executables but not
> with batch files. For example, it works with Mathematica but not with
> maxima, which is launched by a batch file.
Arrgh, .bat wont ever work this way.
I am now split how to move on - incidentaly, the last patch was concerning
your area - cygwin.
The cygwin calller looks safe, while removing runCommand seemed tricky so I was
about
to kill runCommand from the general code and keep it only #defined for the tiny
part of cygwin code - and leave it up to you.
Now we'll need to ifdef _WIN32 in MathExtern.cpp as well to call
getEnv("COMSPEC").
I am no more sure whether complete elimination of runCommand is worth it.
It looked like reasonable hygiene, but for CAS and cygwin we need to
touch code, which is safe and working without clean substitution code.
Do you have opinion?
Below is preliminary draft fixing issue above, cygwin firewalling and removal
of runCommand for the rest of the code to give an idea what I am ambivalent
about now.
Any feedback welcome,
Pavel
>From ea557b366a7485547a0d73b91545b1150244bb94 Mon Sep 17 00:00:00 2001
From: Pavel Sanda <[email protected]>
Date: Thu, 17 Sep 2026 02:55:40 +0200
Subject: [PATCH 12/14] Run the CAS through Systemcall instead of runCommand
Rerouting to argv form would not work here (unless we rewrite the API
just for this consumer so it deals with stdin/out).
Fortunately we have startscript() which spawns without a shell and its
parsecmd() already understands the redirections these commands use,
including the 2>&1 that octave is invoked with. startscript() reports
an exit code rather than the output, so collect the output in a second
temp file and read it back.
On Windows the command cannot be started directly as maxima ships as
maxima.bat.
Compatibility change here, runCommand() used to return the raw bytes,
while fileContents() decodes them as UTF-8 and to_utf8() encodes them
back: output which is not valid UTF-8 now comes back with replacement
characters, and output containing a NUL byte is truncated there.
Should not be a problem for CAS exchange though.
Tier 01 DiD / argv 04c: CAS via Systemcall, no shell
Assisted-by: Claude Opus 5
---
src/mathed/MathExtern.cpp | 54 ++++++++++++++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 4 deletions(-)
diff --git a/src/mathed/MathExtern.cpp b/src/mathed/MathExtern.cpp
index 531138b358..39d79fe440 100644
--- a/src/mathed/MathExtern.cpp
+++ b/src/mathed/MathExtern.cpp
@@ -39,10 +39,12 @@
#include "support/debug.h"
#include "support/docstream.h"
+#include "support/environment.h"
#include "support/FileName.h"
#include "support/filetools.h"
#include "support/gettext.h"
#include "support/lstrings.h"
+#include "support/Systemcall.h"
#include "support/TempFile.h"
#include "support/textutils.h"
@@ -1136,13 +1138,57 @@ namespace {
ofstream os(cas_tmpfile.toFilesystemEncoding().c_str());
os << data << endl;
os.close();
- string command = cmd + " < "
- + quoteName(cas_tmpfile.toFilesystemEncoding());
+
+ // startscript() reports an exit code, so collect the output of
+ // the CAS in a file as well.
+ unique_ptr<TempFile> outfile(new TempFile("casoutput"));
+ outfile->setAutoRemove(false);
+ FileName const cas_outfile = outfile->name();
+ outfile.reset();
+
+ if (cas_outfile.empty()) {
+ lyxerr << "Warning: cannot create temporary file."
+ << endl;
+ cas_tmpfile.removeFile();
+ return string();
+ }
+
+ string redirect =
+ " < " + quoteName(cas_tmpfile.toFilesystemEncoding())
+ + " > " + quoteName(cas_outfile.toFilesystemEncoding());
lyxerr << "calling: " << cmd
<< "\ninput: '" << data << "'" << endl;
- cmd_ret const ret = runCommand(command);
+ Systemcall one;
+#if defined(_WIN32)
+ // CreateProcess(), and so QProcess, appends .exe to a bare
+ // program name and cannot run a batch file at all, while a CAS
+ // often is one (maxima ships as maxima.bat). So start it the
+ // way runCommand() used to, through the command interpreter,
+ // which is what knows about PATHEXT.
+ // Only the command goes to the interpreter. The redirections
+ // stay outside so that they are still set up by the parent,
+ // and 2>&1 with them, since it has to apply to the file the
+ // output is redirected to.
+ string command = cmd;
+ if (suffixIs(command, "2>&1")) {
+ command = rtrim(command.substr(0, command.size() - 4));
+ redirect += " 2>&1";
+ }
+ one.startscript(Systemcall::Wait,
+ quoteName(getEnv("COMSPEC")) + " /d /c "
+ + quoteName(command) + redirect);
+#else
+ one.startscript(Systemcall::Wait, cmd + redirect);
+#endif
+
+ // Read what the CAS produced whatever its exit code was: a
+ // failing run still prints the diagnostics the callers parse
+ // (octave is invoked with 2>&1 for exactly that reason).
+ string const result = to_utf8(cas_outfile.fileContents("UTF-8"));
+
cas_tmpfile.removeFile();
- return ret.result;
+ cas_outfile.removeFile();
+ return result;
}
size_t get_matching_brace(string const & str, size_t i)
--
2.39.5
>From 66281cd27b7b9831c9ea0481e4b0924c268b1856 Mon Sep 17 00:00:00 2001
From: Pavel Sanda <[email protected]>
Date: Thu, 17 Sep 2026 11:16:21 +0200
Subject: [PATCH 13/14] Move the cygdrive probe off runCommand
This is the last runCommand() caller. It cannot go through argv form
like the rest of the series: init() runs from main() before the LyX
instance and thus before any QCoreApplication exists, so QProcess and
Systemcall are not available at this point. That is why this site used
popen in the first place.
So instead of rewriting the call, move the code to it: the _WIN32 branch
of runCommand() becomes a file static here, with the popen branches of
the other platforms dropped. Nothing about the spawned process changes -
same COMSPEC, same CreateProcess, same pipe handling - so there is
nothing to test on Windows beyond a compile.
The copy is deliberate but temporary: the original is what the next
patch deletes, so the code exists twice for the span of one commit.
That is also why this diff shows an addition rather than a move.
Note that os_win32.cpp is the native Windows build only; a Cygwin build
compiles os_cygwin.cpp, which has no cygdrive query at all.
The input redirection and the 2>&1 handling in the moved code are inert
for the single command we pass. They are kept anyway rather than trimmed,
so that what ships on Windows stays what has been shipping for years.
Tier 01 DiD / argv 04d: cygdrive probe keeps its own spawn code
Assisted-by: Claude Opus 5
---
src/support/os_win32.cpp | 111 +++++++++++++++++++++++++++++++++++++--
1 file changed, 107 insertions(+), 4 deletions(-)
diff --git a/src/support/os_win32.cpp b/src/support/os_win32.cpp
index a10c17ede0..8ca3051268 100644
--- a/src/support/os_win32.cpp
+++ b/src/support/os_win32.cpp
@@ -32,17 +32,21 @@
#include "support/lassert.h"
+#include <cstdio>
#include <cstdlib>
+#include <cstring> // memset
#include <iostream>
#include <vector>
#include <QString>
#include <io.h>
+#include <fcntl.h> // _O_RDONLY
#include <direct.h> // _getdrive
#include <fileapi.h> // GetFinalPathNameByHandle
#include <shlobj.h> // SHGetFolderPath
#include <windef.h>
+#include <windows.h>
#include <shellapi.h>
#include <shlwapi.h>
@@ -89,6 +93,104 @@ bool windows_style_tex_paths_ = true;
string cygdrive = "/cygdrive";
+
+// Run cmd through COMSPEC and return its standard output in result,
+// the return value tells whether the command exited successfully.
+//
+// This is the former support::runCommand() with its non-_WIN32 branches
+// dropped, moved here unchanged when that wrapper was removed: init()
+// runs before the QCoreApplication exists, so Systemcall is not usable
+// at this point, and the cygdrive query below is the only caller left.
+//
+// The input redirection and the 2>&1 handling are inert for that one
+// command, but they are kept so this stays byte for byte what has been
+// shipping for years. Do not add callers and do not grow this into a
+// general command runner; that is what Systemcall is for.
+bool runComspec(string const & cmd, string & result)
+{
+ result.clear();
+
+ if (verbose)
+ lyxerr << "\nRunning: " << cmd << endl;
+ else
+ LYXERR(Debug::INFO,"Running: " << cmd);
+
+ STARTUPINFO startup;
+ PROCESS_INFORMATION process;
+ SECURITY_ATTRIBUTES security;
+ HANDLE in, out;
+ FILE * inf = 0;
+ bool err2out = false;
+ string command;
+ string const infile = trim(split(cmd, command, '<'), " \"");
+ command = rtrim(command);
+ if (suffixIs(command, "2>&1")) {
+ command = rtrim(command, "2>&1");
+ err2out = true;
+ }
+ string const cmdarg = "/d /c \"" + command + "\"";
+ string const comspec = getEnv("COMSPEC");
+
+ security.nLength = sizeof(SECURITY_ATTRIBUTES);
+ security.bInheritHandle = TRUE;
+ security.lpSecurityDescriptor = NULL;
+
+ if (CreatePipe(&in, &out, &security, 0)) {
+ memset(&startup, 0, sizeof(STARTUPINFO));
+ memset(&process, 0, sizeof(PROCESS_INFORMATION));
+
+ startup.cb = sizeof(STARTUPINFO);
+ startup.dwFlags = STARTF_USESTDHANDLES;
+
+ startup.hStdError = err2out ? out : GetStdHandle(STD_ERROR_HANDLE);
+ startup.hStdInput = infile.empty()
+ ? GetStdHandle(STD_INPUT_HANDLE)
+ : CreateFile(infile.c_str(), GENERIC_READ,
+ FILE_SHARE_READ, &security, OPEN_EXISTING,
+ FILE_ATTRIBUTE_NORMAL, NULL);
+ startup.hStdOutput = out;
+
+ if (startup.hStdInput != INVALID_HANDLE_VALUE &&
+ CreateProcess(comspec.c_str(), (LPTSTR)cmdarg.c_str(),
+ &security, &security, TRUE, CREATE_NO_WINDOW,
+ 0, 0, &startup, &process)) {
+
+ CloseHandle(process.hThread);
+ int fno = _open_osfhandle((intptr_t)in, _O_RDONLY);
+ CloseHandle(out);
+ inf = _fdopen(fno, "r");
+ }
+ }
+
+ if (!inf) {
+ lyxerr << "RunCommand: could not start child process" << endl;
+ return false;
+ }
+
+ int c = fgetc(inf);
+ while (c != EOF) {
+ result += static_cast<char>(c);
+ c = fgetc(inf);
+ }
+
+ WaitForSingleObject(process.hProcess, INFINITE);
+ DWORD pret;
+ BOOL success = GetExitCodeProcess(process.hProcess, &pret);
+ bool valid = (pret == 0) && success;
+ if (!success)
+ pret = -1;
+ if (!infile.empty())
+ CloseHandle(startup.hStdInput);
+ CloseHandle(process.hProcess);
+ if (fclose(inf) != 0)
+ pret = -1;
+
+ if (pret == -1)
+ perror("RunCommand: could not terminate child process");
+
+ return valid;
+}
+
BOOL terminate_handler(DWORD event)
{
if (event == CTRL_CLOSE_EVENT
@@ -221,8 +323,9 @@ void init(int argc, char ** argv[])
RegCloseKey(hkey);
string const mount = string(buf) + "\\bin\\mount.exe";
if (retval == ERROR_SUCCESS && FileName(mount).exists()) {
- cmd_ret const p =
- runCommand(mount + " --show-cygdrive-prefix");
+ string result;
+ bool const valid = runComspec(
+ mount + " --show-cygdrive-prefix", result);
// The output of the mount command is as follows:
// Prefix Type Flags
// /cygdrive system binmode
@@ -230,8 +333,8 @@ void init(int argc, char ** argv[])
// to the outer split which sets cygdrive with its
// contents until the first blank, discarding the
// unneeded return value.
- if (p.valid && prefixIs(p.result, "Prefix"))
- split(split(p.result, '\n'), cygdrive, ' ');
+ if (valid && prefixIs(result, "Prefix"))
+ split(split(result, '\n'), cygdrive, ' ');
}
}
--
2.39.5
>From c2ef2e6ad419a1c754fe0516bc7d199a4bfe2f75 Mon Sep 17 00:00:00 2001
From: Pavel Sanda <[email protected]>
Date: Thu, 17 Sep 2026 11:18:49 +0200
Subject: [PATCH 14/14] Remove the popen based runCommand() wrapper
Nothing calls it any more. Every consumer has been moved to
Systemcall in the previous patches, and the one site that could
not be - the cygdrive query, which runs before Qt exists - now carries
its own copy of the Windows spawn code.
On unix that is the last shell gone: nothing reaches /bin/sh any more,
so a quoting mistake in a caller cannot turn into command execution.
On Windows two callers still go through COMSPEC, and deliberately so,
because CreateProcess() cannot run a batch file: the cygdrive query
above and the CAS in MathExtern. Both hand it a command of our own -
a literal, or a path to a script we ship - never anything that came
from a document or from the preferences.
The _WIN32 include block in filetools.cpp went with it, it was there
only for this function.
Tier 01 DiD / argv 05: popen wrapper gone, no shell left on unix
Assisted-by: Claude Opus 5
---
src/support/filetools.cpp | 117 --------------------------------------
src/support/filetools.h | 7 ---
2 files changed, 124 deletions(-)
diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp
index d35e4a701b..0fff08f5f8 100644
--- a/src/support/filetools.cpp
+++ b/src/support/filetools.cpp
@@ -63,10 +63,6 @@
#include <QCryptographicHash>
-#if defined (_WIN32)
-#include <io.h>
-#include <windows.h>
-#endif
using namespace std;
@@ -1101,119 +1097,6 @@ bool readLink(FileName const &, FileName &)
#endif
-cmd_ret const runCommand(string const & cmd)
-{
- // FIXME: replace all calls to RunCommand with ForkedCall
- // (if the output is not needed) or the code in ISpell.cpp
- // (if the output is needed).
-
- // One question is if we should use popen or
- // create our own popen based on fork, exec, pipe
- // of course the best would be to have a
- // pstream (process stream), with the
- // variants ipstream, opstream
-
- if (verbose)
- lyxerr << "\nRunning: " << cmd << endl;
- else
- LYXERR(Debug::INFO,"Running: " << cmd);
-
-#if defined (_WIN32)
- STARTUPINFO startup;
- PROCESS_INFORMATION process;
- SECURITY_ATTRIBUTES security;
- HANDLE in, out;
- FILE * inf = 0;
- bool err2out = false;
- string command;
- string const infile = trim(split(cmd, command, '<'), " \"");
- command = rtrim(command);
- if (suffixIs(command, "2>&1")) {
- command = rtrim(command, "2>&1");
- err2out = true;
- }
- string const cmdarg = "/d /c \"" + command + "\"";
- string const comspec = getEnv("COMSPEC");
-
- security.nLength = sizeof(SECURITY_ATTRIBUTES);
- security.bInheritHandle = TRUE;
- security.lpSecurityDescriptor = NULL;
-
- if (CreatePipe(&in, &out, &security, 0)) {
- memset(&startup, 0, sizeof(STARTUPINFO));
- memset(&process, 0, sizeof(PROCESS_INFORMATION));
-
- startup.cb = sizeof(STARTUPINFO);
- startup.dwFlags = STARTF_USESTDHANDLES;
-
- startup.hStdError = err2out ? out : GetStdHandle(STD_ERROR_HANDLE);
- startup.hStdInput = infile.empty()
- ? GetStdHandle(STD_INPUT_HANDLE)
- : CreateFile(infile.c_str(), GENERIC_READ,
- FILE_SHARE_READ, &security, OPEN_EXISTING,
- FILE_ATTRIBUTE_NORMAL, NULL);
- startup.hStdOutput = out;
-
- if (startup.hStdInput != INVALID_HANDLE_VALUE &&
- CreateProcess(comspec.c_str(), (LPTSTR)cmdarg.c_str(),
- &security, &security, TRUE, CREATE_NO_WINDOW,
- 0, 0, &startup, &process)) {
-
- CloseHandle(process.hThread);
- int fno = _open_osfhandle((intptr_t)in, _O_RDONLY);
- CloseHandle(out);
- inf = _fdopen(fno, "r");
- }
- }
-#elif defined (HAVE_POPEN)
- FILE * inf = ::popen(cmd.c_str(), os::popen_read_mode());
-#elif defined (HAVE__POPEN)
- FILE * inf = ::_popen(cmd.c_str(), os::popen_read_mode());
-#else
-#error No popen() function.
-#endif
-
- // (Claus Hentschel) Check if popen was successful ;-)
- if (!inf) {
- lyxerr << "RunCommand: could not start child process" << endl;
- return { false, string() };
- }
-
- string result;
- int c = fgetc(inf);
- while (c != EOF) {
- result += static_cast<char>(c);
- c = fgetc(inf);
- }
-
-#if defined (_WIN32)
- WaitForSingleObject(process.hProcess, INFINITE);
- DWORD pret;
- BOOL success = GetExitCodeProcess(process.hProcess, &pret);
- bool valid = (pret == 0) && success;
- if (!success)
- pret = -1;
- if (!infile.empty())
- CloseHandle(startup.hStdInput);
- CloseHandle(process.hProcess);
- if (fclose(inf) != 0)
- pret = -1;
-#elif defined (HAVE_PCLOSE)
- int const pret = pclose(inf);
- bool const valid = (WEXITSTATUS(pret) == 0);
-#elif defined (HAVE__PCLOSE)
- int const pret = _pclose(inf);
- bool const valid = (WEXITSTATUS(pret) == 0);
-#else
-#error No pclose() function.
-#endif
-
- if (pret == -1)
- perror("RunCommand: could not terminate child process");
-
- return { valid, result };
-}
-
FileName const findtexfile(string const & fil, string const & /*format*/,
bool const onlykpse)
diff --git a/src/support/filetools.h b/src/support/filetools.h
index dc9e5877e7..f65198c2c6 100644
--- a/src/support/filetools.h
+++ b/src/support/filetools.h
@@ -336,13 +336,6 @@ bool prefs2prefs(FileName const & filename, FileName const & tempfile,
/// Does file \p file need to be updated by configure.py?
bool configFileNeedsUpdate(std::string const & file);
-struct cmd_ret {
- bool valid;
- std::string result;
-};
-
-cmd_ret const runCommand(std::string const & cmd);
-
int fileLock(const char * lock_file);
void fileUnlock(int fd, const char * lock_file);
--
2.39.5
--
lyx-devel mailing list
[email protected]
https://lists.lyx.org/mailman/listinfo/lyx-devel
