Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
Oh, nice. Thank you. I was starting to wonder what happened to this.
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
Nikolay Kudryavtsev writes: > Anyway, I've tried to get it to work using shell-quote-argument, see the > attached patch. Seems to work well enough in practice on both platforms > and for cases like (setq temporary-file-directory "/tmp/`echo hi`/"). > From 47690d14ac4838d8e39f08bd8224f0b4af053359 Mon Sep 17 00:00:00 2001 > From: Nikolay Kudryavtsev > Date: Sun, 26 Dec 2021 22:47:19 +0300 > Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows Thanks for the patch, and sorry for the late reply. I have applied your patch onto main adding TINYCHANGE cookie to the commit message. https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6156b57bdf2b14ccfbf6646e8d217599738b694d You are also listed as an Org contributor now. https://git.sr.ht/~bzg/worg/commit/6a5bccb1aeca8e2b3ecef64238002bcfb46f68e0 -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
Nikolay, I do not have any objections concerning the last version of your patch (I have not test it though). It is tracked on https://updates.orgmode.org/ however it may take months before it will be committed. Have you signed FSF papers related to copyright? See https://orgmode.org/worg/org-contribute.html#copyright There is no "TINYCHANGE" keyword in the commit message and I have not found you in the list of contributors to Org mode (Bastien likely have access to more actual data). Since several your patches have been accepted to Emacs, you are close to the limit when formalities become necessary. Do not consider the comments below as a request to change anything in the patch. I am just trying to express more clear what I wrote in the previous message. On 31/12/2021 03:54, Nikolay Kudryavtsev wrote: When some external data is substituted into a Maxima command (batchload this case) there should be an extra pass of escaping that protects special characters like quotes (and backslashes?) accordingly to Maxima rules. Not necessarily, Maxima is capable of understanding unescaped paths, for example, this works: maxima --very-quiet -r "batchload(\"/tmp/sp ce/babel-gxqTkM/maxima-ua3e9j.max\")"$ Consider a really peculiar path for tmp files with quotes /tmp/")$ something-weird$ "/maxima-ua3e9j.max I hope, it is unrealistic and will be properly escaped by %S formatter. I am a bit surprised that Maxima does not allow to do it in a more reliable way since several interfaces exist (WxMaxima, texmacs). Unsure if get_application_args is suitable https://sourceforge.net/p/maxima/mailman/message/35180908/ Command line Maxima actually has a batch flag, but using it returns the entire input file in the output too and that seems to be the reason why the original authors of ob-maxima didn't use it. It would be great if it had another flag to suppress printing content of the batch script or a flag that changes a setting that controls such behavior.
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
When some external data is substituted into a Maxima command (batchload this case) there should be an extra pass of escaping that protects special characters like quotes (and backslashes?) accordingly to Maxima rules. Not necessarily, Maxima is capable of understanding unescaped paths, for example, this works: maxima --very-quiet -r "batchload(\"/tmp/sp ce/babel-gxqTkM/maxima-ua3e9j.max\")"$ I suspect that quotes your added around %S must not be used there. Due to them file name appears outside of quotes at all. Yes, good catch. Unsure concerning Maxima but usually it is possible to pass arguments avoiding quoting issues for particular language. Command line Maxima actually has a batch flag, but using it returns the entire input file in the output too and that seems to be the reason why the original authors of ob-maxima didn't use it. It's probably possible to filter that on our side, but such filtering would require extra work, which they probably deemed unnecessary, for such a rather obscure set of use cases. Anyway, I've tried to get it to work using shell-quote-argument, see the attached patch. Seems to work well enough in practice on both platforms and for cases like (setq temporary-file-directory "/tmp/`echo hi`/"). From 47690d14ac4838d8e39f08bd8224f0b4af053359 Mon Sep 17 00:00:00 2001 From: Nikolay Kudryavtsev Date: Sun, 26 Dec 2021 22:47:19 +0300 Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows * ob-maxima.el (org-babel-execute:maxima): Change command line invocation to a one that should work everywhere. --- lisp/ob-maxima.el | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lisp/ob-maxima.el b/lisp/ob-maxima.el index 7b49bb07a..08b586414 100644 --- a/lisp/ob-maxima.el +++ b/lisp/ob-maxima.el @@ -77,8 +77,11 @@ This function is called by `org-babel-execute-src-block'." (result (let* ((cmdline (or (cdr (assq :cmdline params)) "")) (in-file (org-babel-temp-file "maxima-" ".max")) - (cmd (format "%s --very-quiet -r 'batchload(%S)$' %s" -org-babel-maxima-command in-file cmdline))) + (cmd (format "%s --very-quiet -r %s$ %s" +org-babel-maxima-command + (shell-quote-argument + (format "batchload(%S)" in-file)) + cmdline))) (with-temp-file in-file (insert (org-babel-maxima-expand body params))) (message cmd) ;; " | grep -v batch | grep -v 'replaced' | sed '/^$/d' " -- 2.34.1.windows.1
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
On 30/12/2021 01:37, Nikolay Kudryavtsev wrote: If your temporary-file-directory is something like "/tmp/apostrophe'", it would not work currently either. So apostrophe is a very special case here. As for possible evaluation within the double quotes, while this is theoretically possible, user sort of has to go out of his way to trigger it, so the question is whether we should introduce any platform-specific code to mitigate such an obscure case? Then we are also limited by Maxima itself since it has to be able to read that path too and it's very picky when it comes to file paths. I am not a committer, so it is up to the maintainers to decide if your patch is suitable. My intention is to draw attention to the issue, however they may tolerate it. I have not experimented with remote execution of babel code blocks using tramp, so I may be unaware of some specific, e.g. execution using ssh almost certainly assumes shell command and interface with list of arguments may not be available. When some external data is substituted into a Maxima command (batchload this case) there should be an extra pass of escaping that protects special characters like quotes (and backslashes?) accordingly to Maxima rules. I expect that %S formatter does a trick by adding quotes around the string argument and adding backslashes before quote characters and backslashes inside. I suspect that quotes your added around %S must not be used there. Due to them file name appears outside of quotes at all. This error is hidden unless at least a space character presents in temporary directory path. Unsure concerning Maxima but usually it is possible to pass arguments avoiding quoting issues for particular language. A couple of examples with inline code snippets emacs -Q --batch --eval '(message "bl(%S)$" (car argv))' 'a"b\c.txt' sh -c 'printf "bl(%s)\$\n" "$1"' 'sh' 'a"bc\d.txt' Maybe there is a way to pass file name as a separate argument (without combining it with command) to Maxima as well. In my opinion, platform-specific code should be avoided when possible. Even `shell-quote-argument' may be better. I would prefer e.g. `call-process' from info "(elisp) Synchronous Processes" https://www.gnu.org/software/emacs/manual/html_node/elisp/Synchronous-Processes.html , but I am realizing that it may require more changes in babel or even to cause problems with tramp. Double quotes open issues with injection of commands in backticks `rm something`, $variable expansion and other constructs. I hope, `shell-quote-argument' is reliable enough. P.S. https://xkcd.com/327/ Exploits of a Mom
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
If your temporary-file-directory is something like "/tmp/apostrophe'", it would not work currently either. So apostrophe is a very special case here. As for possible evaluation within the double quotes, while this is theoretically possible, user sort of has to go out of his way to trigger it, so the question is whether we should introduce any platform-specific code to mitigate such an obscure case? Then we are also limited by Maxima itself since it has to be able to read that path too and it's very picky when it comes to file paths.
Re: [PATCH] ob-maxima.el: Fix execution on MS Windows
On 27/12/2021 03:18, Nikolay Kudryavtsev wrote: Ob-maxima currently does not work on Windows due to it using single quotes in the Maxima invocation and those not being supported by Windows CMD. After some testing I've found an invocation that seems to work fine on both Windows and Linux. I don't think this patch can cause any real issue, since the string in those quotes is just the temp file path. --- a/lisp/ob-maxima.el +++ b/lisp/ob-maxima.el @@ -77,7 +77,7 @@ This function is called by `org-babel-execute-src-block'." (result (let* ((cmdline (or (cdr (assq :cmdline params)) "")) (in-file (org-babel-temp-file "maxima-" ".max")) - (cmd (format "%s --very-quiet -r 'batchload(%S)$' %s" + (cmd (format "%s --very-quiet -r \"batchload(\\\"%S\\\")\"$ %s" org-babel-maxima-command in-file cmdline))) I do not like original variant, but suggested change makes it unsafe in more cases. `in-file' might contain apostrophe in the case of peculiar path of the directory for temporary files. More characters may be interpreted by BASH inside double quotes. Even docstring for `shell-quote-argument' mentions security issues with the function. Ideally command arguments should be passed as a list to avoid intermediate interpretation by shell at all. Unfortunately gluing strings to make a shell command is used too widely in org code and emacs API encourages such unsafe way.
[PATCH] ob-maxima.el: Fix execution on MS Windows
Hello. I've been playing around with Maxima from time to time. Ob-maxima currently does not work on Windows due to it using single quotes in the Maxima invocation and those not being supported by Windows CMD. After some testing I've found an invocation that seems to work fine on both Windows and Linux. I don't think this patch can cause any real issue, since the string in those quotes is just the temp file path. From b0206d14df8947c831e56eef21fbd38ca88c1fd5 Mon Sep 17 00:00:00 2001 From: Nikolay Kudryavtsev Date: Sun, 26 Dec 2021 22:47:19 +0300 Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows * ob-maxima.el (org-babel-execute:maxima): Change command line invocation to a one that should work everywhere. --- lisp/ob-maxima.el | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lisp/ob-maxima.el b/lisp/ob-maxima.el index 7b49bb07a..512712221 100644 --- a/lisp/ob-maxima.el +++ b/lisp/ob-maxima.el @@ -77,7 +77,7 @@ This function is called by `org-babel-execute-src-block'." (result (let* ((cmdline (or (cdr (assq :cmdline params)) "")) (in-file (org-babel-temp-file "maxima-" ".max")) - (cmd (format "%s --very-quiet -r 'batchload(%S)$' %s" + (cmd (format "%s --very-quiet -r \"batchload(\\\"%S\\\")\"$ %s" org-babel-maxima-command in-file cmdline))) (with-temp-file in-file (insert (org-babel-maxima-expand body params))) (message cmd) -- 2.34.1.windows.1