On Fri, Sep 10, 2021 at 1:32 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <rpl...@apache.org> wrote:
> >
> > On 9/10/21 11:04 AM, Ruediger Pluem wrote:
> > >
> > > On 9/10/21 8:55 AM, Joe Orton wrote:
> > >> On Fri, Sep 10, 2021 at 12:51:53AM -0000, yla...@apache.org wrote:
> > >>>
> > >>> apr_file_setaside: don't blindly kill the old cleanup and file
> > >>> descriptor.
> > >>>
> > >>> There is no cleanup with APR_FOPEN_NOCLEANUP, so
> > >>> apr_pool_cleanup_kill() can
> > >>> go in the !(old_file->flags & APR_FOPEN_NOCLEANUP) block.
> > >>>
> > >>> The file descriptor can't be invalidated either, the file may be split
> > >>> in
> > >>> multiple buckets and setting aside one shouldn't invalidate the others.
> > >>
> > >> Interesting. So is the API claim that:
> > >>
> > >> * @remark After calling this function, old_file may not be used
> > >>
> > >> not really correct, or needs to be weakened?
> > >
> > > I think it is correct, for the reason you state below and the reason I
> > > mentioned,
> > > but I guess we probably need to review in httpd land if we still use the
> > > old_file
> > > afterwards e.g. when buckets have been split.
>
> The fix still belongs in APR though, being able to split and setaside
> file buckets is broken for now.
>
> >
> > I think the old code with setting the file descriptor of old_file to -1,
> > should reveal
> > in a clear way if we still use the old file afterwards. Keeping the file
> > descriptor may
> > lead to subtle failures some time later that are hard to debug.
>
> OK, the semantics of apr_file_setaside() are to take ownership
> (invalidate the old fd) and we probably can't change that.
>
> So we need to fix file_bucket_setaside() somehow, like this?
Well, this rather:
Index: buckets/apr_buckets_file.c
===================================================================
--- buckets/apr_buckets_file.c (revision 1893196)
+++ buckets/apr_buckets_file.c (working copy)
@@ -18,6 +18,7 @@
#include "apr_general.h"
#include "apr_file_io.h"
#include "apr_buckets.h"
+#include "apr_strings.h" /* for apr_pmemdup() */
#if APR_HAS_MMAP
#include "apr_mmap.h"
@@ -218,16 +219,34 @@ static apr_status_t file_bucket_setaside(apr_bucke
apr_file_t *fd = NULL;
apr_file_t *f = a->fd;
apr_pool_t *curpool = apr_file_pool_get(f);
+ apr_status_t rv;
if (apr_pool_is_ancestor(curpool, reqpool)) {
return APR_SUCCESS;
}
+ /* If the file is shared/split this bucket can't take exclusive ownership
+ * with apr_file_setaside() (which invalidates the current/old a->fd,) so
+ * we need to dup in this case.
+ */
+ if (a->refcount.refcount > 1) {
+ rv = apr_file_dup(&fd, f, reqpool);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ a->refcount.refcount--;
+ a = data->data = apr_pmemdup(reqpool, a, sizeof(*a));
+ a->refcount.refcount = 1;
+ }
+ else {
+ rv = apr_file_setaside(&fd, f, reqpool);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ }
if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
a->readpool = reqpool;
}
-
- apr_file_setaside(&fd, f, reqpool);
a->fd = fd;
return APR_SUCCESS;
}
--
