William A. Rowe, Jr. wrote:
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.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'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.
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.[I really think this code
is a bit bogus - I would rather see us check the LastError
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.for confirmation that it was a volume that doesn't support DACLs.]
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 <[EMAIL PROTECTED]> http://www.xbc.nu/brane/
