You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2021/09/10 00:51:53 UTC
svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Author: ylavic
Date: Fri Sep 10 00:51:53 2021
New Revision: 1893204
URL: http://svn.apache.org/viewvc?rev=1893204&view=rev
Log:
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.
Modified:
apr/apr/trunk/file_io/os2/filedup.c
apr/apr/trunk/file_io/unix/filedup.c
apr/apr/trunk/file_io/win32/filedup.c
Modified: apr/apr/trunk/file_io/os2/filedup.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/os2/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
==============================================================================
--- apr/apr/trunk/file_io/os2/filedup.c (original)
+++ apr/apr/trunk/file_io/os2/filedup.c Fri Sep 10 00:51:53 2021
@@ -113,14 +113,12 @@ APR_DECLARE(apr_status_t) apr_file_setas
}
if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
+ apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
+ apr_file_cleanup);
apr_pool_cleanup_register(p, (void *)(*new_file),
apr_file_cleanup,
apr_file_cleanup);
}
- old_file->filedes = -1;
- apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
- apr_file_cleanup);
-
return APR_SUCCESS;
}
Modified: apr/apr/trunk/file_io/unix/filedup.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/unix/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
==============================================================================
--- apr/apr/trunk/file_io/unix/filedup.c (original)
+++ apr/apr/trunk/file_io/unix/filedup.c Fri Sep 10 00:51:53 2021
@@ -179,6 +179,8 @@ APR_DECLARE(apr_status_t) apr_file_setas
(*new_file)->fname = apr_pstrdup(p, old_file->fname);
}
if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
+ apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
+ apr_unix_file_cleanup);
apr_pool_cleanup_register(p, (void *)(*new_file),
apr_unix_file_cleanup,
((*new_file)->flags & APR_INHERIT)
@@ -186,9 +188,6 @@ APR_DECLARE(apr_status_t) apr_file_setas
: apr_unix_child_file_cleanup);
}
- old_file->filedes = -1;
- apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
- apr_unix_file_cleanup);
#ifndef WAITIO_USES_POLL
(*new_file)->pollset = NULL;
#endif
Modified: apr/apr/trunk/file_io/win32/filedup.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
==============================================================================
--- apr/apr/trunk/file_io/win32/filedup.c (original)
+++ apr/apr/trunk/file_io/win32/filedup.c Fri Sep 10 00:51:53 2021
@@ -211,15 +211,13 @@ APR_DECLARE(apr_status_t) apr_file_setas
(*new_file)->fname = apr_pstrdup(p, old_file->fname);
}
if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
+ apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
+ file_cleanup);
apr_pool_cleanup_register(p, (void *)(*new_file),
file_cleanup,
file_cleanup);
}
- old_file->filehand = INVALID_HANDLE_VALUE;
- apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
- file_cleanup);
-
#if APR_FILES_AS_SOCKETS
/* Create a pollset with room for one descriptor. */
/* ### check return codes */
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/10/21 2:51 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Sep 10 00:51:53 2021
> New Revision: 1893204
>
> URL: http://svn.apache.org/viewvc?rev=1893204&view=rev
> Log:
> 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.
Hmm. Is it safe to use the old_file any longer after apr_file_setaside?
The new and the old apr_file_t use the same OS file descriptor. Hence if you read from one the position
in the file also changes for the other. This might not matter in the unbuffered case, but in the buffered
case I would imagine that the buffer and the file descriptor position of apr_file_t which was not used for reading
get out of sync and end up in a mess.
Regards
Rüdiger
>
> Modified:
> apr/apr/trunk/file_io/os2/filedup.c
> apr/apr/trunk/file_io/unix/filedup.c
> apr/apr/trunk/file_io/win32/filedup.c
>
> Modified: apr/apr/trunk/file_io/os2/filedup.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/os2/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
> ==============================================================================
> --- apr/apr/trunk/file_io/os2/filedup.c (original)
> +++ apr/apr/trunk/file_io/os2/filedup.c Fri Sep 10 00:51:53 2021
> @@ -113,14 +113,12 @@ APR_DECLARE(apr_status_t) apr_file_setas
> }
>
> if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> + apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> + apr_file_cleanup);
> apr_pool_cleanup_register(p, (void *)(*new_file),
> apr_file_cleanup,
> apr_file_cleanup);
> }
>
> - old_file->filedes = -1;
> - apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> - apr_file_cleanup);
> -
> return APR_SUCCESS;
> }
>
> Modified: apr/apr/trunk/file_io/unix/filedup.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/unix/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
> ==============================================================================
> --- apr/apr/trunk/file_io/unix/filedup.c (original)
> +++ apr/apr/trunk/file_io/unix/filedup.c Fri Sep 10 00:51:53 2021
> @@ -179,6 +179,8 @@ APR_DECLARE(apr_status_t) apr_file_setas
> (*new_file)->fname = apr_pstrdup(p, old_file->fname);
> }
> if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> + apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> + apr_unix_file_cleanup);
> apr_pool_cleanup_register(p, (void *)(*new_file),
> apr_unix_file_cleanup,
> ((*new_file)->flags & APR_INHERIT)
> @@ -186,9 +188,6 @@ APR_DECLARE(apr_status_t) apr_file_setas
> : apr_unix_child_file_cleanup);
> }
>
> - old_file->filedes = -1;
> - apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> - apr_unix_file_cleanup);
> #ifndef WAITIO_USES_POLL
> (*new_file)->pollset = NULL;
> #endif
>
> Modified: apr/apr/trunk/file_io/win32/filedup.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/filedup.c?rev=1893204&r1=1893203&r2=1893204&view=diff
> ==============================================================================
> --- apr/apr/trunk/file_io/win32/filedup.c (original)
> +++ apr/apr/trunk/file_io/win32/filedup.c Fri Sep 10 00:51:53 2021
> @@ -211,15 +211,13 @@ APR_DECLARE(apr_status_t) apr_file_setas
> (*new_file)->fname = apr_pstrdup(p, old_file->fname);
> }
> if (!(old_file->flags & APR_FOPEN_NOCLEANUP)) {
> + apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> + file_cleanup);
> apr_pool_cleanup_register(p, (void *)(*new_file),
> file_cleanup,
> file_cleanup);
> }
>
> - old_file->filehand = INVALID_HANDLE_VALUE;
> - apr_pool_cleanup_kill(old_file->pool, (void *)old_file,
> - file_cleanup);
> -
> #if APR_FILES_AS_SOCKETS
> /* Create a pollset with room for one descriptor. */
> /* ### check return codes */
>
>
>
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jan 7, 2022 at 3:58 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/7/22 3:02 PM, Yann Ylavic wrote:
> >
> > Should we be consistent here then, and always use the passed in reqpool?
> > I.e. something like the below (on the current code):
>
> Maybe a good idea.
r1896812.
Thanks;
Yann.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/7/22 3:02 PM, Yann Ylavic wrote:
> On Fri, Sep 10, 2021 at 5:49 PM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Fri, Sep 10, 2021 at 4:44 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>> On 9/10/21 4:04 PM, Yann Ylavic wrote:
>>>> Index: buckets/apr_buckets_file.c
>>>> ===================================================================
>>>> --- buckets/apr_buckets_file.c (revision 1893196)
>>>> +++ buckets/apr_buckets_file.c (working copy)
>>>
>>>> @@ -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;
>>>
>>> Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool, reqpool) like in the else branch?
>>
>> Good question..
>> Since we created a new apr_bucket_file and apr_file_t above with the
>> given reqpool's lifetime, I thought the reads would use it too just
>> like apr_bucket_file_make() uses the given pool.
>>
>> But I don't really understand in the first place why we need to keep
>> the existing ->readpool if it's an ancestor of the given reqpool.
>> It's been so from the very beginning of the reqpool parameter
>> (r58312!), but if one wants to setaside on a subpool it may not be
>> thread-safe to keep using the parent pool.
But if we we setaside on a subpool of readpool where the fd pool is also an ancestor of the subpool
like readpool == fd->pool, then file_bucket_setaside is a noop, because of the first if. Hence I think
it is not made to setaside in subpools, but only to pools which may have an unconnected lifeccyle.
>> While calling apr_file_setaside (or apr_file_dup now) makes sure that
>> the (new) file has the requested lifetime, why use the parent pool for
>> mmaping or !XTHREAD reopening the file?
>
> Should we be consistent here then, and always use the passed in reqpool?
> I.e. something like the below (on the current code):
Maybe a good idea.
Regards
Rüdiger
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 10, 2021 at 5:49 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 4:44 PM Ruediger Pluem <rp...@apache.org> wrote:
> >
> > On 9/10/21 4:04 PM, Yann Ylavic wrote:
> > > Index: buckets/apr_buckets_file.c
> > > ===================================================================
> > > --- buckets/apr_buckets_file.c (revision 1893196)
> > > +++ buckets/apr_buckets_file.c (working copy)
> >
> > > @@ -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;
> >
> > Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool, reqpool) like in the else branch?
>
> Good question..
> Since we created a new apr_bucket_file and apr_file_t above with the
> given reqpool's lifetime, I thought the reads would use it too just
> like apr_bucket_file_make() uses the given pool.
>
> But I don't really understand in the first place why we need to keep
> the existing ->readpool if it's an ancestor of the given reqpool.
> It's been so from the very beginning of the reqpool parameter
> (r58312!), but if one wants to setaside on a subpool it may not be
> thread-safe to keep using the parent pool.
> While calling apr_file_setaside (or apr_file_dup now) makes sure that
> the (new) file has the requested lifetime, why use the parent pool for
> mmaping or !XTHREAD reopening the file?
Should we be consistent here then, and always use the passed in reqpool?
I.e. something like the below (on the current code):
Index: buckets/apr_buckets_file.c
===================================================================
--- buckets/apr_buckets_file.c (revision 1896509)
+++ buckets/apr_buckets_file.c (working copy)
@@ -239,7 +239,6 @@ static apr_status_t file_bucket_setaside(apr_bucke
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;
@@ -246,11 +245,9 @@ static apr_status_t file_bucket_setaside(apr_bucke
}
else {
apr_file_setaside(&fd, f, reqpool);
- if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
- a->readpool = reqpool;
- }
}
a->fd = fd;
+ a->readpool = reqpool;
return APR_SUCCESS;
}
>
> Regards;
> Yann.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 10, 2021 at 4:44 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 9/10/21 4:04 PM, Yann Ylavic wrote:
> > Index: buckets/apr_buckets_file.c
> > ===================================================================
> > --- buckets/apr_buckets_file.c (revision 1893196)
> > +++ buckets/apr_buckets_file.c (working copy)
>
> > @@ -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;
>
> Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool, reqpool) like in the else branch?
Good question..
Since we created a new apr_bucket_file and apr_file_t above with the
given reqpool's lifetime, I thought the reads would use it too just
like apr_bucket_file_make() uses the given pool.
But I don't really understand in the first place why we need to keep
the existing ->readpool if it's an ancestor of the given reqpool.
It's been so from the very beginning of the reqpool parameter
(r58312!), but if one wants to setaside on a subpool it may not be
thread-safe to keep using the parent pool.
While calling apr_file_setaside (or apr_file_dup now) makes sure that
the (new) file has the requested lifetime, why use the parent pool for
mmaping or !XTHREAD reopening the file?
Regards;
Yann.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/10/21 4:04 PM, Yann Ylavic wrote:
> Index: buckets/apr_buckets_file.c
> ===================================================================
> --- buckets/apr_buckets_file.c (revision 1893196)
> +++ buckets/apr_buckets_file.c (working copy)
> @@ -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;
Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool, reqpool) like in the else branch?
> +
> + 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;
> }
Regards
Rüdiger
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 10, 2021 at 3:48 PM Ruediger Pluem <rp...@apache.org> 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.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/10/21 2:55 PM, Yann Ylavic wrote:
> On Fri, Sep 10, 2021 at 1:32 PM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <rp...@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, ylavic@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));
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?
Something like
a = apr_bucket_alloc(sizeof(*a), data->list);
*a = *(data->data); // or memcpy(a, data->data, sizeof(*a))
data->data = a;
Otherwise looks good.
And you would need to bring back
old_file->filedes = -1
as a partial revert of r1893204?
Regards
Rüdiger
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 10, 2021 at 1:32 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <rp...@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, ylavic@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;
}
--
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 10, 2021 at 11:12 AM Ruediger Pluem <rp...@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, ylavic@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?
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.
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
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, ylavic@apache.org wrote:
>>> Author: ylavic
>>> Date: Fri Sep 10 00:51:53 2021
>>> New Revision: 1893204
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1893204&view=rev
>>> Log:
>>> 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.
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.
Regards
Rüdiger
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/10/21 8:55 AM, Joe Orton wrote:
> On Fri, Sep 10, 2021 at 12:51:53AM -0000, ylavic@apache.org wrote:
>> Author: ylavic
>> Date: Fri Sep 10 00:51:53 2021
>> New Revision: 1893204
>>
>> URL: http://svn.apache.org/viewvc?rev=1893204&view=rev
>> Log:
>> 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.
>
> old_file is also modified/invalidated in the ->buffered case where
> old_file->thlock is removed.
>
Regards
Rüdiger
Re: svn commit: r1893204 - in /apr/apr/trunk/file_io: os2/filedup.c
unix/filedup.c win32/filedup.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Sep 10, 2021 at 12:51:53AM -0000, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Sep 10 00:51:53 2021
> New Revision: 1893204
>
> URL: http://svn.apache.org/viewvc?rev=1893204&view=rev
> Log:
> 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?
old_file is also modified/invalidated in the ->buffered case where
old_file->thlock is removed.
Regards, Joe