Skip to content

Conversation

zielmicha
Copy link
Contributor

On Windows put it in double quotes and escape double quotes using backslash.
On Posix put it in single quotes and escape single quotes using '"'"'.

This commit changes what quoteIfContainsWhite does, but before that change it was used incorrectly all over standard library, which caused security issues.

@dom96
Copy link
Contributor

dom96 commented Dec 2, 2013

I'm not sure this is the correct solution. quoteIfContainsWhite is not a proc exclusively intended for osproc, perhaps you should leave quoteIfContainsWhite alone and introduce a new proc with your new implementation.

@zielmicha
Copy link
Contributor Author

This might be better idea. However this will also require changing 27 places where it is used with intent of quoting shell command and will leave original one without any uses.

@zielmicha
Copy link
Contributor Author

I mean, if we leave quoteIfContainsWhite as is, people will keep using it as shell escaping routine, leading to security issues.

By the way, #701 is related.

@Araq
Copy link
Member

Araq commented Dec 2, 2013

I agree with zielmicha. In fact, I consider it a simple oversight of quoteIfContainsWhite. So please fix quoteIfContainsWhite and document its new behaviour and also document this change in web/news.txt.

@zielmicha
Copy link
Contributor Author

Added info to web/news.txt.

…to shell.

On Windows put it in double quotes and escape double quotes using backslash.
On Posix put it in single quotes and escape single quotes using '"'"'.

This commit changes what quoteIfContainsWhite does, but before that change it
was used incorrectly all over standard library, which caused security issues.
Araq added a commit that referenced this pull request Dec 7, 2013
Make quoteIfContainsWhite quote argument, so it can be safely passed to shell.
@Araq Araq merged commit 887466d into nim-lang:master Dec 7, 2013
@gradha
Copy link
Contributor

gradha commented Dec 8, 2013

This change seems to break rst2html invoked during koch web:

nimrod rst2html --compileonly --hint[Conf]:off --putenv:nimrodversion=0.9.3 --path:/Users/gradha/project/nimrod/root -o:web/index.temp web/index.txt
Error: arguments can only be given if the '--run' option is selected

@zielmicha
Copy link
Contributor Author

Ok, I think it breaks the use of [] in arguments - I just had the same problem with compiler.

I think I know what's wrong: argument parser must be using quoteIfWhite and then checks for "--", because it fails in a same way when given parameter containing spaces (on older 0.9.2)

gradha added a commit that referenced this pull request Dec 10, 2013
Reverts "Make quoteIfContainsWhite quote…". Refs #702.
reactormonk pushed a commit to reactormonk/nim that referenced this pull request Apr 7, 2014
Make quoteIfContainsWhite quote argument, so it can be safely passed to shell.
reactormonk pushed a commit to reactormonk/nim that referenced this pull request Apr 7, 2014
This reverts commit a54ba4c to avoid
tool breakage. A different approach is being worked on nim-lang#730.
reactormonk pushed a commit to reactormonk/nim that referenced this pull request Apr 7, 2014
Reverts "Make quoteIfContainsWhite quote…". Refs nim-lang#702.
Clyybber pushed a commit to Clyybber/Nim that referenced this pull request Sep 16, 2023
## Summary

Internal refactor replacing `sem.newSymG` with `newSymGNode`, no change
in actual behaviour

## Details

This replaces the `newSymG`, which produced a symbol and effected
errors. `newSymGNode` will return an `nkSym` node on succees and
`nkError` if it failed. The `getDefNameSymOrRecover` proc can be used in
conjunction in order to ensure progress can be made in the presence of
errors.

All call sites updated in such a fashion are now more ready to be
converted to a more `nkError` aware style.

---------

Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

4 participants