edbrowse-dev - development list for edbrowse
 help / color / mirror / Atom feed
* [Edbrowse-dev]  wordexp
@ 2015-02-14  1:51 Karl Dahlke
  0 siblings, 0 replies; 6+ messages in thread
From: Karl Dahlke @ 2015-02-14  1:51 UTC (permalink / raw)
  To: Edbrowse-dev

This is called to expand variables in file names that the user types in.
I really don't think it should be a problem.

Karl Dahlke

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Edbrowse-dev] wordexp
@ 2015-02-14  0:47 Chris Brannon
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Brannon @ 2015-02-14  0:47 UTC (permalink / raw)
  To: edbrowse-dev

I recently learned that the wordexp function may have security issues on
some operating systems.
http://www.openwall.com/lists/oss-security/2015/02/11/3

So my question is this.  Are we passing any untrusted input to wordexp?
Or does it just expand filenames obtained from the user at the keyboard?

-- Chris

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Edbrowse-dev] wordexp
  2014-12-30 11:10 Karl Dahlke
@ 2014-12-30 13:55 ` Adam Thompson
  0 siblings, 0 replies; 6+ messages in thread
From: Adam Thompson @ 2014-12-30 13:55 UTC (permalink / raw)
  To: Karl Dahlke; +Cc: Edbrowse-dev

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Tue, Dec 30, 2014 at 06:10:49AM -0500, Karl Dahlke wrote:
> After considerable waffling, I decided to give wordexp a try
> for variable expansion and globbing on file names.
> Odds are, we won't notice any difference.
> But at least it is somewhat standard now, and I got rid of a bunch of
> #ifdef Windows dos unix code.
> Some of the old code is still in there under #if 0,
> and I'll get rid of that when I have confidence in the new system.

Ok, everything appears to work ok, though I pushed a change to allow expansion of env variables etc for non-existant file names when writing or setting the file name.
For example:
export tmpdir=/tmp
then:
edbrowse
f $tmpdir/test.txt
w $tmpdir/test_2.txt

Now allows you to set the file name to /tmp/test.txt and write to /tmp/test_2.txt
rather than erroring as it did previously.
Note that, if the file doesn't exist, b,
r and e commands still error as expected.

> Speaking of old stuff, we'll have to decide when to delete
> jsdom.cpp jsloc.cpp js.h.
> That is, when are we committed to the new system?
> Of course, thanks to git, you never really delete stuff,
> you can always get it back if you want it.

Thus I'd say delete it now, since it appears to work just as well as the
previous system.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Edbrowse-dev] wordexp
@ 2014-12-30 11:10 Karl Dahlke
  2014-12-30 13:55 ` Adam Thompson
  0 siblings, 1 reply; 6+ messages in thread
From: Karl Dahlke @ 2014-12-30 11:10 UTC (permalink / raw)
  To: Edbrowse-dev

After considerable waffling, I decided to give wordexp a try
for variable expansion and globbing on file names.
Odds are, we won't notice any difference.
But at least it is somewhat standard now, and I got rid of a bunch of
#ifdef Windows dos unix code.
Some of the old code is still in there under #if 0,
and I'll get rid of that when I have confidence in the new system.

Speaking of old stuff, we'll have to decide when to delete
jsdom.cpp jsloc.cpp js.h.
That is, when are we committed to the new system?
Of course, thanks to git, you never really delete stuff,
you can always get it back if you want it.

Karl Dahlke

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Edbrowse-dev] wordexp
  2014-12-28 21:45 Karl Dahlke
@ 2014-12-29  0:25 ` Adam Thompson
  0 siblings, 0 replies; 6+ messages in thread
From: Adam Thompson @ 2014-12-29  0:25 UTC (permalink / raw)
  To: Karl Dahlke; +Cc: Edbrowse-dev

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

On Sun, Dec 28, 2014 at 04:45:34PM -0500, Karl Dahlke wrote:
> After some playing around with wordexp, I'm inclined to leave things alone.
> wordexp just does too much.
It certainly does a lot, but the existing code also does a fair amount,
and will still break on file names if they're not correctly escaped (which your
examples aren't). Also, I had to read the code to tell me how to
escape the meta characters (though I admit I may've just missed that one in the user's guide).
I'm certainly thinking that glob would be a worth while change to make here,
possibly keeping the hand-rolled env variable expansion?
On the subject of that, what happens if I have the following:
testfile=mytest
and the file:
mytest_22.txt
and try:
r $testfile_22.txt
This doesn't work as testfile_22 is not a variable,
and there appears to be no current way to stop expansion (note that we can't
ignore env vars with _ in them as they're legal in posix shell).
Basically what I'm getting at here is that at the moment the code is buggy and
at least if we use wordexp we can say that "your file names must be valid in a
posix shell" or words to that effect and then it becomes fairly clear what's
allowed (simply \ escape anything you're not sure about).
Thus the above example becomes:
r ${testfile}_22.txt
and a file name like:
it'd be unfortunate to get this.txt
becomes:
it\'d\ be\ unfortunate\ to\ get\ this.txt
or:
"it'd be unfortunate to get this.txt"

At the very least we should bring our variable expansion and escape character in line with something vaguely standard (i.e. optional braces for ambiguous cases in env variables and \ rather than `) imho. We should also probably use glob for... well... globbing I think.
I'm also not confident there aren't more bugs there if someone pokes it hard 
enough.
Having said all that, it'd probably be a nice idea to have a toggle command to
switch off file name expansion for when you really don't need it and have some
horrible file name to edit.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Edbrowse-dev] wordexp
@ 2014-12-28 21:45 Karl Dahlke
  2014-12-29  0:25 ` Adam Thompson
  0 siblings, 1 reply; 6+ messages in thread
From: Karl Dahlke @ 2014-12-28 21:45 UTC (permalink / raw)
  To: Edbrowse-dev

After some playing around with wordexp, I'm inclined to leave things alone.
wordexp just does too much.
Unbalanced parens or brackets or quotes or apostrophes
will produce a syntax error.
Here's a filename with one apostrophe in it.

#include <stdio.h>
#include <wordexp.h>
main()
{
wordexp_t w;
int rc;
rc = wordexp("isn't", &w, 0);
if(rc == WRDE_SYNTAX)
puts("syntax error");
}

And even if the quotes are balanced, they will be taken away from you.

#include <stdio.h>
#include <wordexp.h>
main()
{
wordexp_t w;
int rc;
rc = wordexp("is'n't", &w, 0);
puts(w.we_wordv[0]);
}

So many side effects that I'm sure people will be confused.
I already wrote the code and it's been working for years,
guess I'll leave it alone.

Karl Dahlke

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-02-14  1:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-14  1:51 [Edbrowse-dev] wordexp Karl Dahlke
  -- strict thread matches above, loose matches on Subject: below --
2015-02-14  0:47 Chris Brannon
2014-12-30 11:10 Karl Dahlke
2014-12-30 13:55 ` Adam Thompson
2014-12-28 21:45 Karl Dahlke
2014-12-29  0:25 ` Adam Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).