Re: NSBundle -initWithPath: warning

[Richard Frith-Macdonald]

On 2005-04-01 10:30:26 +0100 David Ayers <d.ayers@inode.at> wrote:

> Sheldon Gill wrote:
> 
>> 
>> Why is the code checking for an absolute path? It is superfluous and is 
>> more likely to cause problems than solve them.
> 
> I kind of agree that this seems superfluous.  If there is a reason, we need 
> some documentation.

The check is not superfluous ... and is already documented both in the Apple 
documentation and in the GNUstep base documentation.  However, the behavior as 
a result of the check is undocumented ... though it should be of course.  
Reading the documentation conservatively/literally I think that the code should 
return nil as a result of the check ... however GNUstep has a history of being 
permissive, and the current automatic conversion from relative to absolute path 
might be considered a good thing.  Certainly we should not be stricter is 
MacOS-X is not.  I will test the behavior on MacOS-X and make the GNUstep 
behavior strict if MacOS-X is strict.

>> Later on, it checks that the bundle directory is readable. Why?
> 
> This may have a reason wrt to setuid programs.

I think that's a digression onto another topic ... the Apple documentation 
specifically states that the method will return nil if the directory is not 
accessible and the GNUstep documentation says it will return nil if the 
directory doesn't exist.  The code here is necessary to conform to spec, 
regardless of whatever merits it may or may not have.

  A file may be detected as non 
> readable/writable because the real uid's don't match but actually is 
> readable/writable because the actual access is done with the effective uid.
> 
> I'm not 100% sure about this behavior and whether we should be checking or 
> not.  I've had warnings that defaults couldn't be created because of this, 
> even though they could have.  But I'm not clear about the intention of this 
> feature within GNUstep in relation to access()
> 
> ....
>        #include <unistd.h>
> 
>        int access(const char *pathname, int mode);
> ....
>        The check is done with the process's  real  uid  and  gid,
>        rather  than  with the effective ids as is done when actu­
>        ally attempting an operation.  This is  to  allow  set-UID
>        programs  to  easily determine the invoking user's author­
>        ity.
> ....

I agree that this is an issue we ought to resolve ...  for most processes 
realuid == effectiveuid so there is no problem, but my suspicion is that 
access() is the wrong function to be using in NSFileManager  for those cases 
where realuid != effectiveuid
ie. I think, that NSFileManager's use of access() is a bug in cases where 
realuid!=effectiveuid but fine normally.

>> Surely the better option is to simply make the open calls and handle 
>> failures properly. That much has to be done anyway. Why complicate matters 
>> by doing checks that don't give us any real benefit?
>> 
>> Consider 1: If the path isn't absolute it may still work in some cases. In 
>> the cases it doesn't work, it'll fail to find the file because the path is 
>> incorrect. Just like it'd fail if the path was otherwise incorrect.
>> 
>> Consider 2: if the bundle directory is unreadable, it'll fail when we try 
>> to read it. Try to read it and handle not being able to.
> 
> I don't necessarily disagree, it's just that we should be clear about what we 
> want and document it.

Of course ... but we do need to check the documentation we already have before 
we start saying that we need to document something, and In this case we already 
appear to have that documentation (which isn't to say it can't be improved 
upon).

I *think* this is a case of people not having read the documentation, rather 
than an actual move to change the behavior because it is thought to be wrong in 
some way.  If I'm wrong, I hope my comments have already made it clear that I 
believe in OpenStep/MacOS-X compatibility as the most important factor except 
in very rare cases where the Apple implementation is clearly and obviously 
broken/wrong (this plainly is not one of those exceptions).