Hi @Swordfish90, and thanks for cool-retro-term — we're building a little Linux writing cockpit that uses it as the "screen," so we lean on the command-line flags, and we hit a sharp edge in --workdir handling worth flagging.
getNamedArgument() in app/main.cpp reads the token after a flag without checking that it exists or that it's actually a value:
int index = args.indexOf(name);
return (index != -1) ? args[index + 1] : QString(defaultName);
Two ways this bites for --workdir (its only caller):
- Missing value (out of bounds):
cool-retro-term --workdir with nothing after it makes index + 1 == args.size(), so args[index + 1] reads one past the end of the QStringList. Per QList::operator[]'s contract (i < size()) that's undefined behavior — a debug/assert build aborts on the Q_ASSERT_X, and a release build returns a garbage/empty QString.
- Followed by another flag:
cool-retro-term --workdir --fullscreen sets the working directory to the literal string "--fullscreen".
In both cases the function never reaches its own defaultName fallback (QDir::currentPath()), which looks like the intended safe behavior.
A note in the spirit of honesty: I traced this by reading app/main.cpp rather than from a full GUI run — I couldn't get a Qt6 dev build going on my box to reproduce it live — but the index arithmetic is unambiguous and --workdir is the sole caller, so I'm fairly confident. If it's easy for you to confirm on a real build, all the better.
Expected: if the flag has no usable value, fall back to the default (current dir) instead of reading out of bounds / swallowing the next option.
Suggested fix (one line): bounds-check before indexing —
return (index != -1 && index + 1 < args.size()) ? args[index + 1] : QString(defaultName);
Happy to open a small PR if that'd be useful. Thanks again for the project!
Hi @Swordfish90, and thanks for cool-retro-term — we're building a little Linux writing cockpit that uses it as the "screen," so we lean on the command-line flags, and we hit a sharp edge in
--workdirhandling worth flagging.getNamedArgument()inapp/main.cppreads the token after a flag without checking that it exists or that it's actually a value:Two ways this bites for
--workdir(its only caller):cool-retro-term --workdirwith nothing after it makesindex + 1 == args.size(), soargs[index + 1]reads one past the end of theQStringList. PerQList::operator[]'s contract (i < size()) that's undefined behavior — a debug/assert build aborts on theQ_ASSERT_X, and a release build returns a garbage/emptyQString.cool-retro-term --workdir --fullscreensets the working directory to the literal string"--fullscreen".In both cases the function never reaches its own
defaultNamefallback (QDir::currentPath()), which looks like the intended safe behavior.A note in the spirit of honesty: I traced this by reading
app/main.cpprather than from a full GUI run — I couldn't get a Qt6 dev build going on my box to reproduce it live — but the index arithmetic is unambiguous and--workdiris the sole caller, so I'm fairly confident. If it's easy for you to confirm on a real build, all the better.Expected: if the flag has no usable value, fall back to the default (current dir) instead of reading out of bounds / swallowing the next option.
Suggested fix (one line): bounds-check before indexing —
Happy to open a small PR if that'd be useful. Thanks again for the project!