On Fri, Sep 10, 2021 at 3:48 PM Ruediger Pluem <[email protected]> wrote:
>
> On 9/10/21 2:55 PM, Yann Ylavic wrote:
> >
> > Well, this rather:
> >
>
> Don't you need to create the memory for a / data->data via
> apr_bucket_alloc(sizeof(*a), data->list)
> like we do in apr_bucket_file_make for this data structure?
Yes, I had that fixed already in my local patch (see attached).
>
> Otherwise looks good.
> And you would need to bring back
>
> old_file->filedes = -1
>
> as a partial revert of r1893204?
Yes, that was the plan.
Thanks!
Yann.
Index: buckets/apr_buckets_file.c
===================================================================
--- buckets/apr_buckets_file.c (revision 1893196)
+++ buckets/apr_buckets_file.c (working copy)
@@ -212,9 +212,9 @@ APR_DECLARE(apr_status_t) apr_bucket_file_set_buf_
return APR_SUCCESS;
}
-static apr_status_t file_bucket_setaside(apr_bucket *data, apr_pool_t *reqpool)
+static apr_status_t file_bucket_setaside(apr_bucket *b, apr_pool_t *reqpool)
{
- apr_bucket_file *a = data->data;
+ apr_bucket_file *a = b->data;
apr_file_t *fd = NULL;
apr_file_t *f = a->fd;
apr_pool_t *curpool = apr_file_pool_get(f);
@@ -223,11 +223,33 @@ APR_DECLARE(apr_status_t) apr_bucket_file_set_buf_
return APR_SUCCESS;
}
- if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
- a->readpool = reqpool;
+ /* If the file is shared/split accross multiple buckets, this bucket can't
+ * take exclusive ownership with apr_file_setaside() (thus invalidating the
+ * current/old a->fd), let's apr_file_dup() in this case instead.
+ */
+ if (a->refcount.refcount > 1) {
+ apr_bucket_file *new;
+ apr_status_t rv;
+
+ rv = apr_file_dup(&fd, f, reqpool);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ new = apr_bucket_alloc(sizeof(*new), b->list);
+ memcpy(new, a, sizeof(*new));
+ new->refcount.refcount = 1;
+ new->readpool = reqpool;
+
+ a->refcount.refcount--;
+ a = b->data = new;
}
-
- apr_file_setaside(&fd, f, reqpool);
+ else {
+ apr_file_setaside(&fd, f, reqpool);
+ if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
+ a->readpool = reqpool;
+ }
+ }
a->fd = fd;
return APR_SUCCESS;
}
Index: file_io/os2/filedup.c
===================================================================
--- file_io/os2/filedup.c (revision 1893204)
+++ file_io/os2/filedup.c (working copy)
@@ -120,5 +120,6 @@ APR_DECLARE(apr_status_t) apr_file_setaside(apr_fi
apr_file_cleanup);
}
+ old_file->filedes = -1;
return APR_SUCCESS;
}
Index: file_io/unix/filedup.c
===================================================================
--- file_io/unix/filedup.c (revision 1893204)
+++ file_io/unix/filedup.c (working copy)
@@ -188,6 +188,7 @@ APR_DECLARE(apr_status_t) apr_file_setaside(apr_fi
: apr_unix_child_file_cleanup);
}
+ old_file->filedes = -1;
#ifndef WAITIO_USES_POLL
(*new_file)->pollset = NULL;
#endif
Index: file_io/win32/filedup.c
===================================================================
--- file_io/win32/filedup.c (revision 1893204)
+++ file_io/win32/filedup.c (working copy)
@@ -218,6 +218,7 @@ APR_DECLARE(apr_status_t) apr_file_setaside(apr_fi
file_cleanup);
}
+ old_file->filehand = INVALID_HANDLE_VALUE;
#if APR_FILES_AS_SOCKETS
/* Create a pollset with room for one descriptor. */
/* ### check return codes */
