On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <[email protected]> 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, [email protected] 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?
Index: buckets/apr_buckets_file.c
===================================================================
--- buckets/apr_buckets_file.c (revision 1893196)
+++ buckets/apr_buckets_file.c (working copy)
@@ -218,18 +218,29 @@ 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 (!apr_pool_is_ancestor(a->readpool, reqpool)) {
- a->readpool = reqpool;
+ /* If the current file is shared/split or buffered this bucket can't take
+ * ownership (apr_file_setaside invalidates the current/old a->fd), so we
+ * need to dup in this case.
+ */
+ if (a->refcount.refcount > 1 || apr_file_buffer_size_get(f) > 0) {
+ rv = apr_file_dup(&fd, f, reqpool);
}
-
- apr_file_setaside(&fd, f, reqpool);
- a->fd = fd;
- return APR_SUCCESS;
+ else {
+ rv = apr_file_setaside(&fd, f, reqpool);
+ }
+ if (rv == APR_SUCCESS) {
+ if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
+ a->readpool = reqpool;
+ }
+ a->fd = fd;
+ }
+ return rv;
}
--
Regards;
Yann.
