apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@xbc.nu>
Subject Re: cvs commit: apr/file_io/win32 filestat.c
Date Fri, 08 Feb 2002 00:25:57 GMT
William A. Rowe, Jr. wrote:

>>brane       02/02/06 16:57:21
>>
>>  Modified:    file_io/win32 filestat.c
>>  Log:
>>  Even on NT, a file can be without a DACL -- for example, if it's in a
>>  FAT volume.  In that case, the access rights are effectively 0777,
>>  modulo the readonly bit -- just like they're computed for Win9x.
>>  
>>  Before this change, apr_file_info_get would return APR_INCOMPLETE
>>  if APR_FILE_PROT was requested for such files.
>>
>
>Generally good and interesting patch ;) see my notes below for some required
>bits to round this patch out.
>
>
>>  Revision  Changes    Path
>>  1.63      +23 -17    apr/file_io/win32/filestat.c
>>  
>>  Index: filestat.c
>>  ===================================================================
>>  RCS file: /home/cvs/apr/file_io/win32/filestat.c,v
>>  retrieving revision 1.62
>>  retrieving revision 1.63
>>  diff -u -r1.62 -r1.63
>>  --- filestat.c 1 Feb 2002 01:40:38 -0000 1.62
>>  +++ filestat.c 7 Feb 2002 00:57:21 -0000 1.63
>>
>
>Note that we are always testing APR_FINFO_PROT - the test was duplicitous...
>
Hmm, yes I see the transformation made in apr_file_info_get before 
calling more_finfo. But assuming that the only missing bits could be 
APR_FINFO_PROT is imho not safe. Besides, the intent of the code is more 
obvious this way. I'd leave them in.

>
>
>>  +    if (apr_os_level < APR_WIN_NT)
>>  +        guess_protection_bits(finfo);
>>       else if (wanted & (APR_FINFO_PROT | APR_FINFO_OWNER))
>>       {
>>           /* On NT this request is incredibly expensive, but accurate.
>>  @@ -296,6 +300,8 @@
>>               /* Retrieved the discresionary access list */
>>               resolve_prot(finfo, wanted, dacl);
>>           }
>>  +        else if (wanted & APR_FINFO_PROT)
>>  +            guess_protection_bits(finfo);
>>       }
>>   
>>       return ((wanted & ~finfo->valid) ? APR_INCOMPLETE : APR_SUCCESS);
>>
>
>We don't need to 'ask' if the user wants the guess bits here.  If we have the
>data [don't need another kernel call] then provide the answer.
>
>In this case, we've already tried for a DACL, come up empty, so we are going to
>use the readonly bits to spit out a pseudo-answer.
>
I'm not sure I follow this. Earlier in the code, we have

        if (wanted & APR_FINFO_PROT)
            sinf |= DACL_SECURITY_INFORMATION;


and we also ask (wanted & APR_FINFO_PROT) to decide whether to pass the 
pointer to dacl to Get(Named)SecurityInfo. If we don't request the DACL 
from the system, we can't guess the protection bits based on its absence.

> [I really think this code
>
>is a bit bogus - I would rather see us check the LastError
>
Yeah, I see now that the return code is effectively ignored (missed that 
last night, due to it beng 2:30 a.m.). Very bogus indeed, as that's 
/another/ thing to check before deciding whether the "guessed" prot bits 
would be valid or not.

> for confirmation that it was a volume that doesn't support DACLs.]
>
Nope. Even files on an NTFS volume can be without a DACL; that happened 
to me. If we request it, and the request succeeds, and dacl is NULL, we 
know everything.

If the request does /not/ succeed, we know nothing. But then we should 
be returning an appropriate error code. What's the use of telling the 
caller that nobody has any access rights to the file, if in fact the 
caller isn't allowed to query the security info?

>  In any case, give them the answer
>and set the APR_FINFO_PROT bits since we don't need another system call.
>
>Bill
>
I'd propose one of these two changes:

--- filestat.c.~1.63.~	Thu Feb  7 01:42:36 2002
+++ filestat.c	Fri Feb  8 01:23:39 2002
@@ -284,7 +284,7 @@
             apr_pool_cleanup_register(finfo->cntxt, pdesc, free_localheap, 
                                  apr_pool_cleanup_null);
         else
-            user = grp = dacl = NULL;
+            return APR_FROM_OS_ERROR(rv);
 
         if (user) {
             finfo->user = user;

or:

--- filestat.c.~1.63.~	Thu Feb  7 01:42:36 2002
+++ filestat.c	Fri Feb  8 01:24:51 2002
@@ -300,7 +300,7 @@
             /* Retrieved the discresionary access list */
             resolve_prot(finfo, wanted, dacl);
         }
-        else if (wanted & APR_FINFO_PROT)
+        else if (rv == ERROR_SUCCESS && (wanted & APR_FINFO_PROT))
             guess_protection_bits(finfo);
     }
 

I'd prefer the first one, but can live with the second.


-- 
Brane ─îibej   <brane@xbc.nu>   http://www.xbc.nu/brane/




Mime
View raw message