i#2097: In drfront_searchenv on Unix, do not search current directory.

Commit log for first patchset:
---------------
i#2097: In drfront_searchenv on Unix, do not search current directory.

Also in drfront_searchenv on Unix, handle empty paths, which refer
to the current directory. Likewise, modify drfront_get_app_full_path
and _tmain in drdeploy.c to behave in the Unix manner on Unix.

Fixes #2097
---------------

[bruening, 2016-12-10 23:13:36]

> Also in drfront_searchenv on Unix, handle empty paths, which refer
> to the current directory.

I don't think we should do this: this is not POSIX behavior.

https://codereview.appspot.com/313210043/diff/1/libutil/dr_frontend_unix.c
File libutil/dr_frontend_unix.c (left):

https://codereview.appspot.com/313210043/diff/1/libutil/dr_frontend_unix.c#ol...
libutil/dr_frontend_unix.c:145: if (realpath(fname, realpath_buf) != NULL) {
So this routine will now fail on an explicit relative or an absolute fname. 
Please clarify that in the header comments.  Hopefully this won't break any
existing users.  It looks like DrMem calls drfront_get_app_full_path() for the
app and only uses this searchenv routine directly to find notepad.

https://codereview.appspot.com/313210043/diff/1/libutil/dr_frontend_unix.c#ol...
libutil/dr_frontend_unix.c:319: buf[buflen - 1] = '\0';
I suppose removing this defensive code is safe b/c it's our own code being
called and it guarantees null-termination

https://codereview.appspot.com/313210043/diff/1/libutil/dr_frontend_unix.c
File libutil/dr_frontend_unix.c (right):

https://codereview.appspot.com/313210043/diff/1/libutil/dr_frontend_unix.c#ne...
libutil/dr_frontend_unix.c:149: "%.*s%s%s", (int)(next - cur), cur, next == cur
? "" : "/", fname);
This looks wrong: I don't see how next can ever equal cur.

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c
File tools/drdeploy.c (right):

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1493
tools/drdeploy.c:1493: #if defined(UNIX)
style: # if

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1497
tools/drdeploy.c:1497: full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
style violation: {}

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1497
tools/drdeploy.c:1497: full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
Safest to follow the practice of all the other code here and NULL_TERMINATE
after the call

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1500
tools/drdeploy.c:1500: full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
style violation: {}

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1500
tools/drdeploy.c:1500: full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
Ditto

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1500
tools/drdeploy.c:1500: full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
It seems like these could both be replaced by a single call to get_full_path()
which calls drfront_get_app_full_path(), which would be a cross-platform
solution and would match what drmemory's frontend does.  We would then get rid
of the #ifdef UNIX.

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1502
tools/drdeploy.c:1502: error("cannot find executable");
Include the name of the executable

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1502
tools/drdeploy.c:1502: error("cannot find executable");
This is a behavior change vs the prior code, and a change vs Windows: best to
have all platforms behave the same.  Presumably in the current code it continues
and tries to exec the unqualified name and hits some error message below (maybe
a more useful message, which one is it?) or maybe there's some chance it will
work?

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1506
tools/drdeploy.c:1506: #elif defined(WINDOWS)
style

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1521
tools/drdeploy.c:1521: BUFFER_SIZE_ELEMENTS(full_app_name));
_wsearchenv searches . so this may not be needed.  Can we re-unify the two (like
the code was), with no ifdef: simpler and easier to maintain with a single code
path when they are pretty similar (except the +.exe).  See above comment about
get_full_path().

https://codereview.appspot.com/313210043/diff/1/tools/drdeploy.c#newcode1526
tools/drdeploy.c:1526: #else
style, here and next few lines

[Edmund.Grimley.Evans, 2016-12-11 08:12:51]

On 2016/12/10 23:13:36, bruening wrote:
> > Also in drfront_searchenv on Unix, handle empty paths, which refer
> > to the current directory.
> 
> I don't think we should do this: this is not POSIX behavior.

But you want "bin64/drrun -- command" to do the same thing as "command", right?

http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html says: "A
zero-length prefix is a legacy feature that indicates the current working
directory.". So the feature's there, but it's recommended not to use it.
However, some users will have an empty component in their PATH. If we're going
to do something different from the shell we should warn them... We could give
them an error message, insisting that they update their PATH before trying to
use DynamoRIO again...

> tools/drdeploy.c:1502: error("cannot find executable");
> This is a behavior change vs the prior code, and a change vs Windows:

The aim of the patch is to make "bin64/drrun -- command" do the same thing as
"command" on the OS the user is currently using, rather than simulate the
behaviour of one OS on a different OS.

So, if there's an executable "xx" in the current directory, but the current
directory is not on the PATH, then both "xx" and "bin64/drrun -- xx" will fail.
Bash says "bash: xx: command not found". Dash says "dash: 1: xx: not found".
Perhaps I should make drrun's error more similar to the shell's error...

[bruening, 2016-12-11 22:02:32]

On 2016/12/11 08:12:51, Edmund.Grimley.Evans wrote:
> > tools/drdeploy.c:1502: error("cannot find executable");
> > This is a behavior change vs the prior code, and a change vs Windows:
> 
> The aim of the patch is to make "bin64/drrun -- command" do the same thing as
> "command" on the OS the user is currently using, rather than simulate the
> behaviour of one OS on a different OS.

My complaint is that an error about not finding the app to run should be in the
same point in the drdeploy code with the same error handling logic, rather than
completely split where in the code different platforms find out about whether
the app is valid.  Having it split makes it hard to maintain and modify other
aspects of what the code is doing if some platforms have bailed out already
while others reach further on the same type of error.

[bruening, 2016-12-12 03:57:10]

On 2016/12/11 08:12:51, Edmund.Grimley.Evans wrote:
> On 2016/12/10 23:13:36, bruening wrote:
> > > Also in drfront_searchenv on Unix, handle empty paths, which refer
> > > to the current directory.
> > 
> > I don't think we should do this: this is not POSIX behavior.
> 
> But you want "bin64/drrun -- command" to do the same thing as "command",
right?
> 
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html says:
"A
> zero-length prefix is a legacy feature that indicates the current working
> directory.". So the feature's there, but it's recommended not to use it.
> However, some users will have an empty component in their PATH. If we're going
> to do something different from the shell we should warn them... We could give
> them an error message, insisting that they update their PATH before trying to
> use DynamoRIO again...

OK, sure, let's support it if it's actually used.  I am not familiar with it and
thought it was some esoteric legacy thing that maybe wasn't even supported in
most modern shells.

[bruening, 2016-12-12 03:58:17]

On 2016/12/11 22:02:32, bruening wrote:
> On 2016/12/11 08:12:51, Edmund.Grimley.Evans wrote:
> > > tools/drdeploy.c:1502: error("cannot find executable");
> > > This is a behavior change vs the prior code, and a change vs Windows:
> > 
> > The aim of the patch is to make "bin64/drrun -- command" do the same thing
as
> > "command" on the OS the user is currently using, rather than simulate the
> > behaviour of one OS on a different OS.
> 
> My complaint is that an error about not finding the app to run should be in
the
> same point in the drdeploy code with the same error handling logic, rather
than
> completely split where in the code different platforms find out about whether
> the app is valid.  Having it split makes it hard to maintain and modify other
> aspects of what the code is doing if some platforms have bailed out already
> while others reach further on the same type of error.

Somewhere there is an existing error message about a not-found app which is now
dead code: seems like some kind of change to that should happen if a new earlier
error is put in place.

[Edmund.Grimley.Evans, 2016-12-12 12:57:15]

On 2016/12/12 03:58:17, bruening wrote:
> Somewhere there is an existing error message about a not-found app which is
now
> dead code: seems like some kind of change to that should happen if a new
earlier
> error is put in place.

I've not found that error message yet...

[bruening, 2016-12-12 14:31:29]

On 2016/12/12 12:57:15, Edmund.Grimley.Evans wrote:
> On 2016/12/12 03:58:17, bruening wrote:
> > Somewhere there is an existing error message about a not-found app which is
> now
> > dead code: seems like some kind of change to that should happen if a new
> earlier
> > error is put in place.
> 
> I've not found that error message yet...

% bin64/drrun -- foo
ERROR: Failed to create process for "/work/dr/git/build_x64_dbg_tests/foo":

[Edmund.Grimley.Evans, 2016-12-12 14:50:28]

On 2016/12/12 14:31:29, bruening wrote:
> On 2016/12/12 12:57:15, Edmund.Grimley.Evans wrote:
> > On 2016/12/12 03:58:17, bruening wrote:
> > > Somewhere there is an existing error message about a not-found app which
is
> > now
> > > dead code: seems like some kind of change to that should happen if a new
> > earlier
> > > error is put in place.
> > 
> > I've not found that error message yet...
> 
> % bin64/drrun -- foo
> ERROR: Failed to create process for "/work/dr/git/build_x64_dbg_tests/foo":

That's a bit different, though. In that case it has a path to an executable but
has failed to run the executable: there's probably no file there. The case I'm
concerned with is that there is an executable called "foo" in the current
directory, but the current directory isn't on the path, so we mustn't even try
to run it.

[Edmund.Grimley.Evans, 2016-12-12 14:53:03]

On 2016/12/12 14:50:28, Edmund.Grimley.Evans wrote:
> On 2016/12/12 14:31:29, bruening wrote:
> > On 2016/12/12 12:57:15, Edmund.Grimley.Evans wrote:
> > > On 2016/12/12 03:58:17, bruening wrote:
> > > > Somewhere there is an existing error message about a not-found app which
> is
> > > now
> > > > dead code: seems like some kind of change to that should happen if a new
> > > earlier
> > > > error is put in place.
> > > 
> > > I've not found that error message yet...
> > 
> > % bin64/drrun -- foo
> > ERROR: Failed to create process for "/work/dr/git/build_x64_dbg_tests/foo":
> 
> That's a bit different, though. In that case it has a path to an executable
but
> has failed to run the executable: there's probably no file there. The case I'm
> concerned with is that there is an executable called "foo" in the current
> directory, but the current directory isn't on the path, so we mustn't even try
> to run it.

Though perhaps I'm being too pedantic. Perhaps it doesn't really matter all that
much if drrun implicitly adds the current working directory to the end of the
PATH. (What we had before, I think, is the current working directory implicitly
added to the start of the PATH, which is a more serious problem.)

[bruening, 2016-12-12 15:31:41]

On 2016/12/12 14:53:03, Edmund.Grimley.Evans wrote:
> On 2016/12/12 14:50:28, Edmund.Grimley.Evans wrote:
> > On 2016/12/12 14:31:29, bruening wrote:
> > > On 2016/12/12 12:57:15, Edmund.Grimley.Evans wrote:
> > > > On 2016/12/12 03:58:17, bruening wrote:
> > > > > Somewhere there is an existing error message about a not-found app
which
> > is
> > > > now
> > > > > dead code: seems like some kind of change to that should happen if a
new
> > > > earlier
> > > > > error is put in place.
> > > > 
> > > > I've not found that error message yet...
> > > 
> > > % bin64/drrun -- foo
> > > ERROR: Failed to create process for
"/work/dr/git/build_x64_dbg_tests/foo":
> > 
> > That's a bit different, though. In that case it has a path to an executable
> but
> > has failed to run the executable: there's probably no file there. The case
I'm
> > concerned with is that there is an executable called "foo" in the current
> > directory, but the current directory isn't on the path, so we mustn't even
try
> > to run it.
> 
> Though perhaps I'm being too pedantic. Perhaps it doesn't really matter all
that
> much if drrun implicitly adds the current working directory to the end of the
> PATH. (What we had before, I think, is the current working directory
implicitly
> added to the start of the PATH, which is a more serious problem.)

I agree with *not* searching the cur dir unless . (or apparently blank) is on
PATH.

But I do think the error message on a not-found or inaccessible path should list
the path it tried (which is very useful), regardless of what the shell would
print, and I also think that the code should be as cross-platform as possible,
sharing as much app locating and error handling as possible.  It looks like the
current error message is when drinjectlib fails, so there's no explicit
drdeploy.c check, so adding an earlier one seems ok if we're sure it's accurate
-- but IMHO it should be on all platforms: UNIX or Linux should not have a
special up front check + error that nobody else has.

[Edmund.Grimley.Evans, 2016-12-13 10:51:27]

On 2016/12/12 15:31:41, bruening wrote:
> On 2016/12/12 14:53:03, Edmund.Grimley.Evans wrote:
> > On 2016/12/12 14:50:28, Edmund.Grimley.Evans wrote:
> > > On 2016/12/12 14:31:29, bruening wrote:
> > > > On 2016/12/12 12:57:15, Edmund.Grimley.Evans wrote:
> > > > > On 2016/12/12 03:58:17, bruening wrote:
> > > > > > Somewhere there is an existing error message about a not-found app
> which
> > > is
> > > > > now
> > > > > > dead code: seems like some kind of change to that should happen if a
> new
> > > > > earlier
> > > > > > error is put in place.
> > > > > 
> > > > > I've not found that error message yet...
> > > > 
> > > > % bin64/drrun -- foo
> > > > ERROR: Failed to create process for
> "/work/dr/git/build_x64_dbg_tests/foo":
> > > 
> > > That's a bit different, though. In that case it has a path to an
executable
> > but
> > > has failed to run the executable: there's probably no file there. The case
> I'm
> > > concerned with is that there is an executable called "foo" in the current
> > > directory, but the current directory isn't on the path, so we mustn't even
> try
> > > to run it.
> > 
> > Though perhaps I'm being too pedantic. Perhaps it doesn't really matter all
> that
> > much if drrun implicitly adds the current working directory to the end of
the
> > PATH. (What we had before, I think, is the current working directory
> implicitly
> > added to the start of the PATH, which is a more serious problem.)
> 
> I agree with *not* searching the cur dir unless . (or apparently blank) is on
> PATH.
> 
> But I do think the error message on a not-found or inaccessible path should
list
> the path it tried (which is very useful), regardless of what the shell would
> print, and I also think that the code should be as cross-platform as possible,
> sharing as much app locating and error handling as possible.  It looks like
the
> current error message is when drinjectlib fails, so there's no explicit
> drdeploy.c check, so adding an earlier one seems ok if we're sure it's
accurate
> -- but IMHO it should be on all platforms: UNIX or Linux should not have a
> special up front check + error that nobody else has.

The normal behaviour on Unix is as follows:

If the "command" contains a '/', then use it as an absolute/relative filepath;
the environment variable PATH is ignored.

Otherwise, search for "command" in each directory listed in PATH.

How does Windows differ from this?

I am aware of the following possible differences:

* Windows uses '\' instead of '/', and PATH is separated with ';' instead of
':'.

* Windows may allow certain suffices to be added to "command". (Is this true
even if "command" already appears to have a suffix?)

* Windows looks in the current directory before it looks at PATH.

* But presumably Windows, just like Unix, doesn't use the PATH if the command
contains a '\'. Can you confirm?

What sort of error message are you expecting in the case where "command" (not
containing '/') is not found? You say it should "list the path it tried". Do you
mean it should print out the value of PATH, or print out every pathname it
tried, based on PATH?

When trying each element of PATH you have to check that the thing is executable
and is an ordinary file. For example, if PATH is "a:b", and a/x is a directory,
or a non-executable file, then typing "x" should cause b/x to be executed, if
b/x is an executable ordinary file. This is an argument for using stat() rather
than access() for this operation. We know we're not setuid - we're the shell, or
we're drrun - so we don't have to worry about that. Since we're not interested
in writability we don't have to worry about read-only file systems. So the
access check used for searching PATH is potentially quite different from the
access check used in some other situations.

[bruening, 2016-12-13 16:49:00]

On Tue, Dec 13, 2016 at 5:51 AM, <Edmund.Grimley.Evans@gmail.com> wrote:

> What sort of error message are you expecting in the case where "command"
> (not containing '/') is not found? You say it should "list the path it
> tried". Do you mean it should print out the value of PATH, or print out
> every pathname it tried, based on PATH?
>

I just want it to print the string that was passed on the command line.  In
some cases there are layers of scripts or variable expansions used that
make it non-trivial for the user to easily see the final string, in which
case having just "drrun: command not found." with no further information is
frustrating.  Additionally, some new users do not put in the -- separator,
have misspelled options, and things get mis-parsed with some option token
interpreted as the app.  Furthermore, on Windows there are UI actions such
as "drag-and-drop" of icons where it may not be clear what is taken as the
file to execute.  Printing the string taken to be the app often points
directly to the problem and saves time.  Lots of reasons to print it, and
no downside.

[bruening, 2016-12-13 17:02:23]

>
> * Windows uses '\' instead of '/', and PATH is separated with ';'
> instead of ':'.
>

/ also works in most places (one place it does not is in cmd shell as
target app path, but even there we need to honor it as we may get paths
coming from cygwin or msys shells).

* Windows may allow certain suffices to be added to "command". (Is this
> true even if "command" already appears to have a suffix?)
>

I don't know about these corner cases, it would take experimenting/docs
research.

* But presumably Windows, just like Unix, doesn't use the PATH if the
> command contains a '\'. Can you confirm?
>

Sounds right, or a / presumably.

[edmund.grimley.evans_gmail.com, 2016-12-13 17:04:09]

On Tuesday, 13 December 2016 16:49:01 UTC, Derek Bruening wrote:
>
> On Tue, Dec 13, 2016 at 5:51 AM, <Edmund.Gri...@gmail.com <javascript:>> 
> wrote:
>
>> What sort of error message are you expecting in the case where "command"
>> (not containing '/') is not found? You say it should "list the path it
>> tried". Do you mean it should print out the value of PATH, or print out
>> every pathname it tried, based on PATH?
>>
>
> I just want it to print the string that was passed on the command line.  
> In some cases there are layers of scripts or variable expansions used that 
> make it non-trivial for the user to easily see the final string, in which 
> case having just "drrun: command not found." with no further information is 
> frustrating.  Additionally, some new users do not put in the -- separator, 
> have misspelled options, and things get mis-parsed with some option token 
> interpreted as the app.  Furthermore, on Windows there are UI actions such 
> as "drag-and-drop" of icons where it may not be clear what is taken as the 
> file to execute.  Printing the string taken to be the app often points 
> directly to the problem and saves time.  Lots of reasons to print it, and 
> no downside.
>

I agree. Even bash does that:

$ foo
bash: foo: command not found
$