Re: archive.apache.org issues

[Ruediger Pluem <rp...@apache.org>]

On 12/31/2006 08:17 PM, William A. Rowe, Jr. wrote:
> Justin Erenkrantz wrote:
> 
>>On 12/30/06, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>>Digging somewhat deeper it turns out that adding APR_FINFO_NAME to the
>>>list of wanted
>>>information causes this apr_stat to return always APR_INCOMPLETE on
>>>Unix platforms in
>>>the case that the call to the native stat / lstat does not fail. This
>>>raises the
>>>following questions:
>>>
>>>1. Do we need to add APR_FINFO_NAME to this apr_stat call? I think we
>>>do not need it
>>>   on Unix platforms but I am not sure if this is true for other
>>>platforms.
>>
>>resolve_symlink seems to preserve it.  *shrug*
>>
>>
>>>2. If we need it can we simply ignore an APR_INCOMPLETE return code on
>>>all platforms
>>>   and only bail out if ((rv != APR_INCOMPLETE) && (rv != APR_SUCCESS))
>>
>>+1.  -- justin
> 
> 
> +/-1 - Accepting APR_INCOMPLETE was an excellent choice.
> 
> But your patch falls one step short.  It's necessary to inspect fstat.valid
> before we use specific fields.

Yes and no. In general you are right: I missed this check. OTH AFAICT provided
that the native OS stat did not fail for some reason (which will cause the OS
error code returned to rv) apr_stat always returns a valid filetype info on all
platforms. But I am unsure if this is guaranteed by the apr API contract, so it might
be safer to check here:


--- server/request.c    (Revision 491297)
+++ server/request.c    (Arbeitskopie)
@@ -563,7 +563,8 @@
                  * is always going to return APR_INCOMPLETE in the case that
                  * the call to the native stat / lstat did not fail.
                  */
-                if ((rv != APR_INCOMPLETE) && (rv != APR_SUCCESS)) {
+                if (((rv != APR_INCOMPLETE) && (rv != APR_SUCCESS))
+                    || (!(thisinfo.valid & APR_FINFO_TYPE))) {
                     /*
                      * This should never happen, because we did a stat on the
                      * same file, resolving a possible symlink several lines


If this patch is not seen as needed it might make sense to extend the comment
above to explain why it is not needed :-).


Regards

Rüdiger