From: Borut R. <bor...@si...> - 2003-06-18 19:29:24
|
Bernhard, I saw that you did some changes in support/Util/MySystem.c. I wrote that peace of code, so I'm curious what went wrong: - you removed quotes surrounding the executable path/name: I added them to support spaces in executable's path/name. I think that this should work (and it also worked for some time ;-), because commands are executed by system() function, which actually pass the parameter as the command line to the shell, which should interpret them in the expected way. What exactly was the problem? - you removed the code which searches *nix dir separators ("/") in function has_path() if compiled on WIN32. I wrote that, because on WIN32 systems both dir separator types are valid ("/" and "\"). Is there a special reason, why you removed it? Borut |
From: Bernhard H. <ber...@be...> - 2003-06-18 20:29:39
|
Hi Borut! > - you removed quotes surrounding the executable path/name: > I added them to support spaces in executable's path/name. > I think that this should work (and it also worked for some time ;-), > because commands are executed by system() function, which actually > pass the parameter as the command line to the shell, which should > interpret them in the expected way. What exactly was the problem? After your commit popen() segfaulted on my box and on the CF. I don't know, which way or why it worked before. My "quick fix" solved the problem. I'm quite sure, that surrounding quotes are not the right way on Unix. There's certainly a right way to escape spaces. I'll dig into this some day. Up to then we can live with my "quick" fix. You'll hardly ever find directory-names or program-names with spaces on Unix. > - you removed the code which searches *nix dir separators ("/") > in function has_path() if compiled on WIN32. I wrote that, because > on WIN32 systems both dir separator types are valid ("/" and "\"). > Is there a special reason, why you removed it? Ahem. has_path() is not used on WIN32 ;-) Now I see, that has_path() is buggy. An absolute path on Unix starts with '/'. - if (strrchr(path, DIR_SEPARATOR_CHAR) == NULL) + if (*path == DIR_SEPARATOR_CHAR) Nevertheless: thanks for your work! Bernhard |
From: Borut R. <bor...@si...> - 2003-06-18 20:44:42
|
> I'm quite sure, that surrounding quotes are not the right way on Unix. There's > certainly a right way to escape spaces. I'll dig into this some day. Up to > then we can live with my "quick" fix. You'll hardly ever find directory-names > or program-names with spaces on Unix. I think that the proper solution would be: * Using spawnvp() instead of system() is more portable cross platform approach, * but then also a substitute for _popen() should be developed... This is what I wrote as a comment in MySystem.c. But I still think that quotes should work. I think that I did some testing before I committed the code... I'll do some more investigations when I'll have some time... > Ahem. has_path() is not used on WIN32 ;-) ;-))))) Obviously it was used once upon a time ;-) > Now I see, that has_path() is buggy. An absolute path on Unix starts with '/'. > - if (strrchr(path, DIR_SEPARATOR_CHAR) == NULL) > + if (*path == DIR_SEPARATOR_CHAR) I don't think so. has_path() doesn't check for the absolute path, but for any path - absolute or relative. So it searches for the path separator anywhere in the string. Borut |
From: Bernhard H. <ber...@be...> - 2003-06-18 20:56:42
|
Hi Borut! > I don't think so. has_path() doesn't check for the absolute path, but for > any path - > absolute or relative. So it searches for the path separator anywhere in the > string. Either the comment or the function has a bug: /*! * check if the path is absolute */ static int has_path(const char *path) { if (strrchr(path, DIR_SEPARATOR_CHAR) == NULL) return 0; return 1; } Bernhard |
From: Borut R. <bor...@si...> - 2003-06-18 21:58:56
|
Hi Bernhard, > /*! > * check if the path is absolute > */ The bug is in the comment :-(( Borut |
From: Tim W. <sdc...@wo...> - 2003-06-18 21:05:13
|
On Wed, 18 Jun 2003, Borut Razem wrote: > > I'm quite sure, that surrounding quotes are not the right way on Unix. > There's > > certainly a right way to escape spaces. I'll dig into this some day. Up to > > then we can live with my "quick" fix. You'll hardly ever find > directory-names > > or program-names with spaces on Unix. > I think that the proper solution would be: > * Using spawnvp() instead of system() is more portable cross platform > approach, > * but then also a substitute for _popen() should be developed... > This is what I wrote as a comment in MySystem.c. > But I still think that quotes should work. I think that I did some testing > before I > committed the code... I'll do some more investigations when I'll have some > time... > Quotes are fine in unix. The other way of escaping spaces is to use '\' eg cd "path with spaces" or cd path\ with\ spaces or even cd 'path with spaces' The double quotes and single quotes differ in the shell variables will be expanded in the double quotes but not in the single quotes: [tim@feynman tim]$ cd /tmp [tim@feynman tmp]$ mkdir "path with spaces" [tim@feynman tmp]$ TESTPATH="path with spaces" [tim@feynman tmp]$ cd "$TESTPATH" [tim@feynman path with spaces]$ cd .. [tim@feynman tmp]$ cd $TESTPATH bash: cd: path: No such file or directory [tim@feynman tmp]$ cd '$TESTPATH' bash: cd: $TESTPATH: No such file or directory [tim@feynman tmp]$ I hope that all makes sense. Regards, Tim. -- God said, "div D = rho, div B = 0, curl E = - @B/@t, curl H = J + @D/@t," and there was light. http://tjw.hn.org/ http://www.locofungus.btinternet.co.uk/ |
From: Bernhard H. <ber...@be...> - 2003-06-18 21:36:26
|
> Quotes are fine in unix. The other way of escaping spaces is to use '\' > > eg cd "path with spaces" > or cd path\ with\ spaces > or even cd 'path with spaces' > > The double quotes and single quotes differ in the shell variables will be > expanded in the double quotes but not in the single quotes: > > [tim@feynman tim]$ cd /tmp > [tim@feynman tmp]$ mkdir "path with spaces" > [tim@feynman tmp]$ TESTPATH="path with spaces" > [tim@feynman tmp]$ cd "$TESTPATH" > [tim@feynman path with spaces]$ cd .. > [tim@feynman tmp]$ cd $TESTPATH > bash: cd: path: No such file or directory > [tim@feynman tmp]$ cd '$TESTPATH' > bash: cd: $TESTPATH: No such file or directory > [tim@feynman tmp]$ > > I hope that all makes sense. I'm afraid it doesn't make sense. While bash removes all your quotes, sdcc uses a library function popen(). Bernhard |
From: Tim W. <sdc...@wo...> - 2003-06-18 21:29:44
|
On Wed, 18 Jun 2003, Borut Razem wrote: > I think that the proper solution would be: > * Using spawnvp() instead of system() is more portable cross platform > approach, I don't think spawnvp() can be portable - I don't know what it does and it doesn't appear in the man pages of linux at least. I suspect it does the equivalent of fork() + execvp() but I'm guessing. Note also that execvp can have differing search order - some systems put the current directory after /bin and /usr/bin as an anti-trojan horse protection, linux doesn't and puts the current directory first as expected. (assuming that $PATH doesn't appear in the environment and execvp uses the default search path) system() is an ANSI C function, execvp and friends are POSIX. Regards, Tim. -- God said, "div D = rho, div B = 0, curl E = - @B/@t, curl H = J + @D/@t," and there was light. http://tjw.hn.org/ http://www.locofungus.btinternet.co.uk/ |
From: Borut R. <bor...@si...> - 2003-06-18 21:59:01
|
> I suspect it does the equivalent of fork() + execvp() but I'm guessing. Yes, this is what I meant. spawnvp() exists on MSVC, and is really not portable. So here is an other buggy comment... > Note also that execvp can have differing search order - some systems > put the current directory after /bin and /usr/bin as an anti-trojan > horse protection, linux doesn't and puts the current directory first > as expected. (assuming that $PATH doesn't appear in the environment > and execvp uses the default search path) I thought that $PATH is taken for the search order. Thank you for the explanation. > system() is an ANSI C function, execvp and friends are POSIX. So, does it mean that we shouldn't use fork() + execvp() and stay with system()? Borut |
From: Tim W. <sdc...@wo...> - 2003-06-18 22:33:26
|
On Wed, 18 Jun 2003, Borut Razem wrote: I'm jumping in on the middle of this without actually looking at the code we are talking about but I'll try and answer as best as I can. First, Bernhard's comment/question about popen/quotes. popen executes it's command as the parameter to /bin/sh -c which means that escaping etc will occur exactly like the shell does - e.g. fpipe = popen("\"/tmp/path with spaces/program\""); or fpipe = popen("/tmp/path\\ with\\ spaces/program"); or fpipe = popen("'/tmp/path with spaces/program'"); or fpipe = popen("$PROGRAM"); will cause the shell to expand $PROGRAM and the execute it (breaking the command/arguments at spaces) fpipe = popen("\"$PROGRAM\""); will cause the shell to expand $PROGRAM but it will be a single argv[0] parameter with any embedded spaces and fpipe = popen("'$PROGRAM'"); will cause the shell to attempt to execute a program called $PROGRAM (All the extra backslashes are so it will compile in C) > > I suspect it does the equivalent of fork() + execvp() but I'm guessing. > Yes, this is what I meant. spawnvp() exists on MSVC, and is really not > portable. > So here is an other buggy comment... > > > Note also that execvp can have differing search order - some systems > > put the current directory after /bin and /usr/bin as an anti-trojan > > horse protection, linux doesn't and puts the current directory first > > as expected. (assuming that $PATH doesn't appear in the environment > > and execvp uses the default search path) > I thought that $PATH is taken for the search order. Thank you for the > explanation. > PATH is used provided it exists. It is the behaviour if it doesn't exist that can change on various systems. It probably isn't a concern to SDCC but it can, potentially, lead to security issues if a user deliberately runs the program without the path set, especially if the app has more privileges than the user has. > > system() is an ANSI C function, execvp and friends are POSIX. > So, does it mean that we shouldn't use fork() + execvp() and stay with > system()? > They achieve different things on unix. The program will block while you are in the system() call so you can't do any processing in parallel while waiting for the result. OTOH, there is no need to do any calling of wait() to clean up any zombie processes. fork() + exec() allows the parent and child to run in parallel. However you do need to remember to cleanup any zombies (or make sure you detach the child - IIRC you have to fork twice and the child immediately suicides leaving the grandchild attached to init rather than the grandparent. I would have to look this up again if you do want to go along this route as it is a while since I have done something like this) To be honest I don't think portability needs to be a big worry over whether you chose fork()+exec() or system(). All unix systems will support the POSIX extensions to C. It is only if you want to start porting to non-unix systems (AS400 or PDAs are the systems most likely not to offer fork() or exec() that spring to mind) that this decision might cause portability issues. Tim. -- God said, "div D = rho, div B = 0, curl E = - @B/@t, curl H = J + @D/@t," and there was light. http://tjw.hn.org/ http://www.locofungus.btinternet.co.uk/ |
From: Bernhard H. <ber...@be...> - 2003-06-18 22:47:06
|
> popen executes it's command as the parameter to /bin/sh -c which means > that escaping etc will occur exactly like the shell does - You're right, I just read the same in the doc. I should run the debugger, and look where the segfault happens ... Bernhard |
From: Bernhard H. <ber...@be...> - 2003-06-19 09:35:35
|
> I should run the debugger, and look where the segfault happens ... I couldn't reproduce the segfault today. As Borut wrote, this code is quite old and it's indeed very unlikely that it caused the problem. I reverted my bad fix and appologize for the confusion, Bernhard |