httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <>
Subject Re: issues
Date Tue, 02 Jan 2007 11:39:12 GMT

On 12/31/2006 08:17 PM, William A. Rowe, Jr. wrote:
> Justin Erenkrantz wrote:
>>On 12/30/06, Ruediger Pluem <> 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
>>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 :-).



View raw message