Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improper argument parsing #959

Closed
jredfox opened this issue Jun 5, 2022 · 7 comments
Closed

improper argument parsing #959

jredfox opened this issue Jun 5, 2022 · 7 comments

Comments

@jredfox
Copy link

jredfox commented Jun 5, 2022

Expected Behavior

executes a command with spaces that is quotes with either ' or "

Current Behavior

parses every space into a new argument regardless of quotes

Possible Solution
Steps to Reproduce (for bugs)

1. java -jar 'my spaced java app.jar'
output:

jredfox@jredfox-lm:~$ qterminal -de java -jar '/home/jredfox/Desktop/red sands.jar'
qt5ct: using qt5ct plugin
Properties constructor called
TRANSLATIONS_DIR: Loading translation file "qterminal_en_US.qm" from dir /usr/share/qterminal/translations
load success: false
qt5ct: D-Bus global menu: no
Canot open file "/home/jredfox/.config/qterminal.org/qterminal_bookmarks.xml"
"Press \"F12\" to see the terminal."
Trying to load translation file from dir "/usr/share/cinnamon"
Trying to load translation file from dir "/usr/share/gnome"
Trying to load translation file from dir "/home/jredfox/.local/share/flatpak/exports/share"
Trying to load translation file from dir "/var/lib/flatpak/exports/share"
Trying to load translation file from dir "/usr/local/share"
Trying to load translation file from dir "/usr/share"
Trying to load translation file from dir "/usr/share/qtermwidget5/translations"
default KB_LAYOUT_DIR:  "/usr/share/qtermwidget5/kb-layouts"
loadAllColorSchemes
Shell program: "java -jar /home/jredfox/Desktop/red sands.jar"
("java", "-jar", "/home/jredfox/Desktop/red", "sands.jar")
Properties destructor called
Context

unable to support your terminal due to it being broken

System Information
  • QTerminal version: 0.14.1
  • Distribution & Version: linux mint amd 64
@tsujan
Copy link
Member

tsujan commented Jun 5, 2022

Definitely not the case with the latest release.

Also, see #958

@tsujan tsujan closed this as completed Jun 5, 2022
@yan12125
Copy link
Member

yan12125 commented Jun 6, 2022

Ancient qterminal version aside, this is indeed a bug.

$ LANG=en_US.UTF-8 strace -e trace=execve -f qterminal -e java -jar '/home/jredfox/Desktop/red sands.jar' |& grep execve
execve("/usr/bin/qterminal", ["qterminal", "-e", "java", "-jar", "/home/jredfox/Desktop/red sands."...], 0x7fff778fc398 /* 103 vars */) = 0
[pid  4139] execve("", ["", "-jar", "/home/jredfox/Desktop/red", "sands.jar"], 0x55aad9adb920 /* 103 vars */) = -1 ENOENT (No such file or directory)

See #335.

@tsujan
Copy link
Member

tsujan commented Jun 6, 2022

With -e yes. Without -e, it's OK:

spaces

@yan12125
Copy link
Member

yan12125 commented Jun 6, 2022

yep

@tsujan
Copy link
Member

tsujan commented Jun 6, 2022

I found a "solution":

I used '\x1' (QChar(01)) instead of space (QLatin1Char(' ')) when getting shell_command in "main.cpp". Then, I got its parts by using:

QStringList parts = shell.split(QChar(01), Qt::SkipEmptyParts);

Instead of

QStringList parts = shell.split(QRegExp(QStringLiteral("\\s+")), Qt::SkipEmptyParts);

In "termwidget.cpp".

It worked here.

However, I think a proper solution may be using of QStringList instead of QString. That requires many changes to the code, for which I don't have time today.

@yan12125
Copy link
Member

yan12125 commented Jun 6, 2022

However, I think a proper solution may be using of QStringList instead of QString

Yeah, I have the same feeling #335 (comment), but I cannot finish it after 5 years :(

@tsujan
Copy link
Member

tsujan commented Jun 6, 2022

Yeah, I have the same feeling #335 (comment), but I cannot finish it after 5 years :(

I understand. Lack of time has also made me overestimate the amount of the required changes and, as a result, I've delayed implementations.

After taking a look at the code, I think the changes are simple in this case. Will make a PR tonight or tomorrow and will request your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants