You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by br...@apache.org on 2002/02/07 01:57:21 UTC
cvs commit: apr/file_io/win32 filestat.c
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.
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
@@ -203,6 +203,25 @@
return rv;
}
+static void guess_protection_bits(apr_finfo_t *finfo)
+{
+ /* Read, write execute for owner. In the Win9x environment, any
+ * readable file is executable (well, not entirely 100% true, but
+ * still looking for some cheap logic that would help us here.)
+ * The same holds on NT if a file doesn't have a DACL (e.g., on FAT)
+ */
+ if (finfo->protection & APR_FREADONLY) {
+ finfo->protection |= APR_WREAD | APR_WEXECUTE;
+ }
+ else {
+ finfo->protection |= APR_WREAD | APR_WEXECUTE | APR_WWRITE;
+ }
+ finfo->protection |= (finfo->protection << prot_scope_group)
+ | (finfo->protection << prot_scope_user);
+
+ finfo->valid |= APR_FINFO_UPROT | APR_FINFO_GPROT | APR_FINFO_WPROT;
+}
+
apr_status_t more_finfo(apr_finfo_t *finfo, const void *ufile,
apr_int32_t wanted, int whatfile)
{
@@ -210,23 +229,8 @@
PACL dacl = NULL;
apr_status_t rv;
- if (apr_os_level < APR_WIN_NT)
- {
- /* Read, write execute for owner. In the Win9x environment, any
- * readable file is executable (well, not entirely 100% true, but
- * still looking for some cheap logic that would help us here.)
- */
- if (finfo->protection & APR_FREADONLY) {
- finfo->protection |= APR_WREAD | APR_WEXECUTE;
- }
- else {
- finfo->protection |= APR_WREAD | APR_WEXECUTE | APR_WWRITE;
- }
- finfo->protection |= (finfo->protection << prot_scope_group)
- | (finfo->protection << prot_scope_user);
-
- finfo->valid |= APR_FINFO_UPROT | APR_FINFO_GPROT | APR_FINFO_WPROT;
- }
+ 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);
Re: cvs commit: apr/file_io/win32 filestat.c
Posted by Branko Čibej <br...@xbc.nu>.
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 <br...@xbc.nu> http://www.xbc.nu/brane/
Re: cvs commit: apr/file_io/win32 filestat.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
> 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...
> + 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 really think this code
is a bit bogus - I would rather see us check the LastError for confirmation that
it was a volume that doesn't support DACLs.] In any case, give them the answer
and set the APR_FINFO_PROT bits since we don't need another system call.
Bill