Is this code reasonable?

[sosij.morris]

The 'spec' is to write a 'whereis' program for Windows. There was no
need to have argv[1] be valid sot hat bit I've added.

The spec also said that to detect an existing file, one could use
anything. As gcc had access() I used that.

Note that I've yet to comment this up - so please don't tell me to do
that - I will.

The question I mostly have is, as this is a short program, is it ok to
have reused 'path'?

Firstly it's assigned the return from getenv(), then the return from
the initial strtok() call, and then it's assigned NULL to keep
strtok() going.

Thanks

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <io.h>

char const * whereis(char const * const prg)
{
static char filepath[MAX_PATH];

char * path;

path = getenv("PATH");

while((path = strtok(path, ";")) != NULL)
{
strncpy(filepath, path, MAX_PATH);

if(path[strlen(path) - 1] != '\\')
{
strncat(filepath, "\\", MAX_PATH);
}

strncat(filepath, prg, MAX_PATH);

if(access(filepath, X_OK) == 0)
{
return filepath;
}

path = NULL;
}

return NULL;
}


int main(int argc, char * argv[])
{
char const * filepath;

if(argc != 2)
{
puts("Arg required");
}

else

{
if((filepath = whereis(argv[1])) != NULL)
{
puts(filepath);
}

else

{
puts("Not found");
}
}

return 0;
}

 

[sosij.morris]

The 'spec' is to write a 'whereis' program for Windows.  There was no
need to have argv[1] be valid sot hat bit I've added.

The spec also said that to detect an existing file, one could use
anything. As gcc had access() I used that.

Note that I've yet to comment this up - so please don't tell me to do
that - I will.

The question I mostly have is, as this is a short program, is it ok to
have reused 'path'?

Firstly it's assigned the return from getenv(), then the return from
the initial strtok() call, and then it's assigned NULL to keep
strtok() going.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <io.h>

char const * whereis(char const * const prg)
{
    static char filepath[MAX_PATH];

     What's MAX_PATH?  Something from <io.h>, maybe?  Is it an
array size or a string length (i.e., is the trailing '\0'
accounted for)?  Does it differ in any important way from the
Standard-defined FILENAME_MAX?

    char * path;

    path = getenv("PATH");

    while((path = strtok(path, ";")) != NULL)

     Two problems here, both potentially fatal.  First, if getenv()
returned a NULL, passing that NULL to strtok() is an error.  Second,
strtok() may modify the pointed-to string, and trying to modify the
string returned by getenv() is also an error.

    {
        strncpy(filepath, path, MAX_PATH);

     You've fallen for the "strncpy() is a safe strcpy()" myth.
Note that strncpy() does not necessarily produce a string: it may
leave `filepath' without a trailing '\0'.

        if(path[strlen(path) - 1] != '\\')
        {
            strncat(filepath, "\\", MAX_PATH);

     The third argument of strncat() doesn't mean what you think
it means.

        }

        strncat(filepath, prg, MAX_PATH);

     The third argument of strncat() doesn't mean what you think
it means.

        if(access(filepath, X_OK) == 0)

        {
            return filepath;
        }

        path = NULL;

     I see nothing objectionable in NULLing the variable for the
next iteration.  My own preference would have been to gather all
the iteration-by-iteration stuff in one place:

        for (  ;  (path = strtok(path, ";")) != NULL;  path = NULL) {
            ...
        }

... but that's only a preference, and yours are allowed to differ.
De gustibus non disputandum est (Latin: "There's no point arguing
with Gus").

    }

    return NULL;
}

     Other remarks: For use on Windows, `whereis waldo' should
probably also look for `waldo.exe' and `waldo.bat' and maybe
`waldo.com' and `waldo.lnk' and a few others.  Also (you should
double-check this) I think Windows will always search the current
directory for executables and things even if they're not in the
PATH, so you might want to look there.

     For further advice on the Windows-specific stuff, I suggest
you try a Windows forum.

Brilliant - thanks!

What's <io.h>? Some kind of Windows-ism, perhaps?

Needed for the predeclaration of access() and X_OK

What's MAX_PATH?

Didn't know about FILENAME_MAX - thanks. In stdlib.h?

Two problems here, both potentially fatal. First, if getenv()
returned a NULL, passing that NULL to strtok() is an error.

Ok, but, I think that Windows will always haven something here - in
the PATH environment variable. So, I belive it's ok for a non-NULL
result for this.

Second, strtok() may modify the pointed-to string,

So, strtok() may modify the original character array path pointed to?
But, as I don't use that anymore; I just assigned the variable 'path'
to whatever strtok returns; is this really a problem?

and trying to modify the string returned by getenv() is also an error.

The getenv() function I have declared says that it returns a char *,
i.e., no const is given, so I thought I could pass that to strtok().
The documentation I have for strtok doesn't say it will try to alter
whatever its passed char * will point to -- in fact I'd have thought,
to be useful, it would make a copy so that it's more 'usuable'.
You're saying this isn't so?

You've fallen for the "strncpy() is a safe strcpy()" myth.
Note that strncpy() does not necessarily produce a string: it may
leave `filepath' without a trailing '\0'.

So I should ensure that filepath is NULL terminated? What's the best
way to do that - make sure there's enough space, AND initialize to all-
zeros?

The third argument of strncat() doesn't mean what you think it means..
?

I guess access() and X_OK are from <io.h>?
Yup.

Other remarks: For use on Windows, `whereis waldo' should
probably also look for `waldo.exe' and `waldo.bat' and maybe

Indeed - I asked about using wildcards, and, failing a 'nod' there,
asked about matching substrings etc. I was 'no'. The spec is to find
the windows file specified in its entirety - whereis ruby should
result in nothing, as should ruby.bat. whereis ruby.exe should match
the first file foound, and at that point stop looking -- multiple
ruby.exes aren't important as Windows searches PATH in-order.

Many thanks, and please correct my 'isn't this ok' thens above.

 

[Martien Verbruggen]

So, strtok() may modify the original character array path pointed to?
But, as I don't use that anymore; I just assigned the variable 'path'
to whatever strtok returns; is this really a problem?

Yes.

path is a pointer, not the string itself. strtok() may modify the string
pointed to by its first argument, and will return a pointer into that
same string on a successful match.

getenv() returns a pointer to the value in the environment, so when you
use strtok() on that, you're modifying that value. It's not a local copy.
In fact, the C99 standard (and the POSIX standard) specifically states
that you're not allowed to modify the string pointed to by the pointer
returned by getenv(), so you shouldn't (as Eric already stated). Make a
local copy.

The getenv() function I have declared says that it returns a char *,
i.e., no const is given, so I thought I could pass that to strtok().

You can never infer the full specification of a function from its
signature. You need to read the documentation that comes with it; in
this case that is the C standard, although your local documentation
should tell you the same thing.

The documentation I have for strtok doesn't say it will try to alter
whatever its passed char * will point to -- in fact I'd have thought,
to be useful, it would make a copy so that it's more 'usuable'.
You're saying this isn't so?

Correct. It isn't so. It modifies what you give it. This is one of the
reasons I never use strtok() to tokenise strings that I need for
something else later on. I only really ever use it to tokenise lines I'm
reading from files. Anything more permanenent in the program I generally
use strpbrk(), strcspn() or strchr() for

Martien