Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
Thx for both of you for the explanation.
Le 06/07/2020 à 11:56, Yann Ylavic a écrit :
[...]
Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
with the current struct apr_bucket_brigade API.
That's also my understanding. That is what was puzzling me.
Just a bit fragile :)
Ok. Better safe than sorry.
CJ
Regards;
Yann.
Passed: apache/httpd#963 (trunk - 023a72b)
Build Update for apache/httpd - Build: #963 Status: Passed Duration: 14 mins and 56 secs Commit: 023a72b (trunk) Author: Graham Leggett Message: Bump the logno. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1879548 13f79535-47bb-0310-9956-ffa450edef68 View the changeset: https://github.com/apache/httpd/compare/92470c0baa38...023a72b97766 View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/705377507?utm_medium=notification_source=email -- You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847_medium=notification_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Canceled: apache/httpd#962 (trunk - 92470c0)
Build Update for apache/httpd - Build: #962 Status: Canceled Duration: 7 mins and 30 secs Commit: 92470c0 (trunk) Author: Ruediger Pluem Message: * Update logno git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1879547 13f79535-47bb-0310-9956-ffa450edef68 View the changeset: https://github.com/apache/httpd/compare/d57cb74a8536...92470c0baa38 View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/705376768?utm_medium=notification_source=email Restart your build: https://travis-ci.org/github/apache/httpd/builds/705376768?utm_medium=notification_source=email -- You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847_medium=notification_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Re: svn commit: r1879546 - in /httpd/httpd/trunk: CHANGES modules/http2/h2_from_h1.c
On 7/6/20 2:13 PM, rpl...@apache.org wrote:
> Author: rpluem
> Date: Mon Jul 6 12:13:33 2020
> New Revision: 1879546
>
> URL: http://svn.apache.org/viewvc?rev=1879546=rev
> Log:
> * Make get_line more robust in the case that it is called multiple times:
>
> - Safe the brigade between mutiple calls to correctly handle transient
> buckets.
> - Detect possible endless loops.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/http2/h2_from_h1.c
>
> Modified: httpd/httpd/trunk/modules/http2/h2_from_h1.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_from_h1.c?rev=1879546=1879545=1879546=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_from_h1.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_from_h1.c Mon Jul 6 12:13:33 2020
> @@ -351,13 +352,17 @@ static apr_status_t get_line(h2_response
> parser->tmp = apr_brigade_create(task->pool, task->c->bucket_alloc);
> }
> status = apr_brigade_split_line(parser->tmp, bb, APR_BLOCK_READ,
> -HUGE_STRING_LEN);
> +len);
> if (status == APR_SUCCESS) {
> --len;
> status = apr_brigade_flatten(parser->tmp, line, );
> if (status == APR_SUCCESS) {
> /* we assume a non-0 containing line and remove trailing crlf. */
> line[len] = '\0';
> +/*
> + * XXX: What to do if there is an LF but no CRLF?
> + * Should we error out?
> + */
> if (len >= 2 && !strcmp(H2_CRLF, line + len - 2)) {
> len -= 2;
> line[len] = '\0';
> @@ -367,10 +372,47 @@ static apr_status_t get_line(h2_response
>task->id, line);
> }
> else {
> +apr_off_t brigade_length;
> +
> +/*
> + * If the brigade parser->tmp becomes longer than our buffer
> + * for flattening we never have a chance to get a complete
> + * line. This can happen if we are called multiple times
> after
> + * previous calls did not find a H2_CRLF and we returned
> + * APR_EAGAIN. In this case parser->tmp (correctly) grows
> + * with each call to apr_brigade_split_line.
> + *
> + * XXX: Currently a stack based buffer of HUGE_STRING_LEN is
> + * used. This means we cannot cope with lines larger than
> + * HUGE_STRING_LEN which might be an issue.
> + */
Any suggestions on both XXX comments above?
Regards
Rüdiger
Still Failing: apache/httpd#960 (trunk - 7cb887b)
Build Update for apache/httpd - Build: #960 Status: Still Failing Duration: 11 mins and 42 secs Commit: 7cb887b (trunk) Author: Ruediger Pluem Message: * Do not try to parse already aborted requests Submitted by: icing git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1879544 13f79535-47bb-0310-9956-ffa450edef68 View the changeset: https://github.com/apache/httpd/compare/d047c6bcd5c9...7cb887b125ad View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/705371697?utm_medium=notification_source=email -- You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847_medium=notification_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Re: svn commit: r1879488 - /httpd/httpd/trunk/server/util_etag.c
On Mon, Jul 6, 2020 at 12:37 PM Graham Leggett wrote:
>
> On 06 Jul 2020, at 09:28, Ruediger Pluem wrote:
>
> +apr_sha1_init();
> +nbytes = sizeof(buf);
> +while ((status = apr_file_read(fd, buf, )) == APR_SUCCESS) {
>
>
> Why do we still use apr_file_read in case we use MMAP? IMHO we should use
> apr_mmap_offset.
> But we would need to consider the amount of memory we map at the same time
> for large files
> in order to avoid exhausting the memory. So I would propose that we create a
> brigade with one
> filebucket and do bucket reads. So like the following code from the default
> handler (excluding the reading):
>
>bb = apr_brigade_create(r->pool, c->bucket_alloc);
>
>e = apr_brigade_insert_file(bb, fd, 0, r->finfo.size, r->pool);
>
> #if APR_HAS_MMAP
>if (d->enable_mmap == ENABLE_MMAP_OFF) {
>(void)apr_bucket_file_enable_mmap(e, 0);
>}
> #endif
>
> Of course that would require to get the finfo in case we open the file
> descriptor on our own.
>
> The reading would then be something like (without error handling):
>
> while (!APR_BRIGADE_EMPTY(bb))
> {
> e = APR_BUCKET_FIRST(bb)
> apr_bucket_read(e, , );
> apr_sha1_update_binary(, str, len);
> apr_bucket_delete(e);
> }
>
>
> Makes much more sense - r1879541.
FWIW, I don't think MMAP-ing sequentially is better/faster than just
read()ing, it could be double work with the (file)system cache.
But code is simpler and EnableMMap off does it well, so +1.
Regards;
Yann.
Broken: apache/httpd#958 (trunk - d047c6b)
Build Update for apache/httpd - Build: #958 Status: Broken Duration: 9 mins and 21 secs Commit: d047c6b (trunk) Author: Graham Leggett Message: Use a brigade instead of direct reads, allow APR to handle MMAP. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1879541 13f79535-47bb-0310-9956-ffa450edef68 View the changeset: https://github.com/apache/httpd/compare/ec6f9bc29de8...d047c6bcd5c9 View the full build log and details: https://travis-ci.org/github/apache/httpd/builds/705345976?utm_medium=notification_source=email -- You can unsubscribe from build emails from the apache/httpd repository going to https://travis-ci.org/account/preferences/unsubscribe?repository=69847_medium=notification_source=email. Or unsubscribe from *all* email updating your settings at https://travis-ci.org/account/preferences/unsubscribe?utm_medium=notification_source=email. Or configure specific recipients for build notifications in your .travis.yml file. See https://docs.travis-ci.com/user/notifications.
Re: svn commit: r1879488 - /httpd/httpd/trunk/server/util_etag.c
On 06 Jul 2020, at 09:28, Ruediger Pluem wrote:
>> +apr_sha1_init();
>> +nbytes = sizeof(buf);
>> +while ((status = apr_file_read(fd, buf, )) == APR_SUCCESS) {
>
> Why do we still use apr_file_read in case we use MMAP? IMHO we should use
> apr_mmap_offset.
> But we would need to consider the amount of memory we map at the same time
> for large files
> in order to avoid exhausting the memory. So I would propose that we create a
> brigade with one
> filebucket and do bucket reads. So like the following code from the default
> handler (excluding the reading):
>
>bb = apr_brigade_create(r->pool, c->bucket_alloc);
>
>e = apr_brigade_insert_file(bb, fd, 0, r->finfo.size, r->pool);
>
> #if APR_HAS_MMAP
>if (d->enable_mmap == ENABLE_MMAP_OFF) {
>(void)apr_bucket_file_enable_mmap(e, 0);
>}
> #endif
>
> Of course that would require to get the finfo in case we open the file
> descriptor on our own.
>
> The reading would then be something like (without error handling):
>
> while (!APR_BRIGADE_EMPTY(bb))
> {
> e = APR_BUCKET_FIRST(bb)
> apr_bucket_read(e, , );
> apr_sha1_update_binary(, str, len);
> apr_bucket_delete(e);
> }
Makes much more sense - r1879541.
Regards,
Graham
—
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On Mon, Jul 6, 2020 at 11:56 AM Yann Ylavic wrote:
> >
> > In the previous code the first condition in the if was always true, and I
> > am not sure what happened with the second condition in
> > case e was the sentinel.
>
> AIUI, dereferencing the SENTINEL is not accessing unreserved/freed
> memory, it's accessing the RING/BRIGADE head (here bb->list the
> placeholder for `struct {apr_bucket *next, *prev;}`) as if it were an
> apr_bucket (given that struct apr_bucket has its own head/placeholder,
> e->type is `sizeof(apr_bucket*)` bytes after bb->list)).
s/`sizeof(apr_bucket*)` bytes/just/
> That's `apr_bucket_alloc_t *bucket_alloc` in struct
> apr_bucket_brigade, so quite unlikely to be _bucket_type_eos.
> Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
> with the current struct apr_bucket_brigade API. Just a bit fragile :)
>
>
> Regards;
> Yann.
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On Mon, Jul 6, 2020 at 8:44 AM Ruediger Pluem wrote:
>
> On 7/5/20 9:29 AM, Christophe JAILLET wrote:
> >
> > Le 14/02/2017 à 17:43, yla...@apache.org a écrit :
> >> @@ -584,9 +584,9 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
> >>apr_bucket_brigade *bb)
> >> {
> >> if (next) {
> >> -apr_bucket *e;
> >> +apr_bucket *e = APR_BRIGADE_LAST(bb);
> >> -if ((e = APR_BRIGADE_LAST(bb)) && APR_BUCKET_IS_EOS(e) &&
> >> next->r) {
> >> +if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e) &&
> >> next->r) {
> >> /* This is only safe because HTTP_HEADER filter is always in
> >>* the filter stack. This ensures that there is ALWAYS a
> >>* request-based filter that we can attach this to. If the
> >
> > Hi,
> >
> > I don't really understand the change above.
> >
> > The commit description is clear. 'ap_pass_brigade' should deal better with
> > empty brigades.
> >
> > However, if the last bucket is an EOS bucket, how could it be a sentinel?
> > My understanding is that this change is a no-op, but I may have missed
> > something.
> >
> > What is the rational for it?
>
> If the brigade is empty then APR_BRIGADE_LAST(bb) points to the sentinel.
> Hence The first condition of the if is not met and we
> jump over the block. If the brigade is not empty we check if the last bucket
> which is now not the sentinel is the eos bucket.
Yeah, the commit replaced e != NULL (always true) with e != SENTINEL
(the relevant test to avoid dereferencing the sentinel in
APR_BUCKET_IS_EOS).
>
> In the previous code the first condition in the if was always true, and I am
> not sure what happened with the second condition in
> case e was the sentinel.
AIUI, dereferencing the SENTINEL is not accessing unreserved/freed
memory, it's accessing the RING/BRIGADE head (here bb->list the
placeholder for `struct {apr_bucket *next, *prev;}`) as if it were an
apr_bucket (given that struct apr_bucket has its own head/placeholder,
e->type is `sizeof(apr_bucket*)` bytes after bb->list)).
That's `apr_bucket_alloc_t *bucket_alloc` in struct
apr_bucket_brigade, so quite unlikely to be _bucket_type_eos.
Finally APR_BUCKET_IS_{EOS,}(e) on an EMPTY brigade is always false
with the current struct apr_bucket_brigade API. Just a bit fragile :)
Regards;
Yann.
Re: svn commit: r1879488 - /httpd/httpd/trunk/server/util_etag.c
On 7/3/20 9:48 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Fri Jul 3 19:48:53 2020
> New Revision: 1879488
>
> URL: http://svn.apache.org/viewvc?rev=1879488=rev
> Log:
> Add MMAP support to ETag generation.
>
> Modified:
> httpd/httpd/trunk/server/util_etag.c
>
> Modified: httpd/httpd/trunk/server/util_etag.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_etag.c?rev=1879488=1879487=1879488=diff
> ==
> --- httpd/httpd/trunk/server/util_etag.c (original)
> +++ httpd/httpd/trunk/server/util_etag.c Fri Jul 3 19:48:53 2020
> @@ -84,6 +88,113 @@ static void etag_end(char *next, const c
> }
>
> /*
> + * Construct a strong ETag by creating a SHA1 hash across the file content.
> + */
> +static char *make_digest_etag(request_rec *r, etag_rec *er, char *vlv,
> + apr_size_t vlv_len, char *weak, apr_size_t weak_len)
> +{
> +apr_sha1_ctx_t context;
> +unsigned char buf[4096]; /* keep this a multiple of 64 */
> +unsigned char digest[APR_SHA1_DIGESTSIZE];
> +apr_file_t *fd = NULL;
> +core_dir_config *cfg;
> +char *etag, *next;
> +
> +apr_size_t nbytes;
> +apr_off_t offset = 0, zero = 0;
> +apr_status_t status;
> +
> +cfg = (core_dir_config *)ap_get_core_module_config(r->per_dir_config);
> +
> +if (er->fd) {
> +fd = er->fd;
> +}
> +else if (er->pathname) {
> +if ((status = apr_file_open(, er->pathname, APR_READ | APR_BINARY,
> +0, r->pool)) != APR_SUCCESS) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(10251)
> + "Make etag: could not open %s", er->pathname);
> +return "";
> +}
> +}
> +if (!fd) {
> +return "";
> +}
> +
> +if ((status = apr_file_seek(fd, APR_CUR, )) != APR_SUCCESS) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(10252)
> + "Make etag: could not seek");
> +if (er->pathname) {
> +apr_file_close(fd);
> +}
> +return "";
> +}
> +
> +if ((status = apr_file_seek(fd, APR_SET, )) != APR_SUCCESS) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(10253)
> + "Make etag: could not seek");
> +if (er->pathname) {
> +apr_file_close(fd);
> +}
> +return "";
> +}
> +
> +#if APR_HAS_MMAP
> +if (cfg->enable_mmap != ENABLE_MMAP_OFF && er->fd &&
> +er->finfo->size >= APR_MMAP_THRESHOLD &&
> +er->finfo->size < APR_MMAP_LIMIT) {
> +apr_mmap_t *mm;
> +
> +if ((status = apr_mmap_create(, er->fd, 0, er->finfo->size,
> +APR_MMAP_READ, r->pool) != APR_SUCCESS)) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(10253)
> + "Make etag: could not mmap");
> +return "";
> +}
> +
> +}
> +#endif
> +
> +apr_sha1_init();
> +nbytes = sizeof(buf);
> +while ((status = apr_file_read(fd, buf, )) == APR_SUCCESS) {
Why do we still use apr_file_read in case we use MMAP? IMHO we should use
apr_mmap_offset.
But we would need to consider the amount of memory we map at the same time for
large files
in order to avoid exhausting the memory. So I would propose that we create a
brigade with one
filebucket and do bucket reads. So like the following code from the default
handler (excluding the reading):
bb = apr_brigade_create(r->pool, c->bucket_alloc);
e = apr_brigade_insert_file(bb, fd, 0, r->finfo.size, r->pool);
#if APR_HAS_MMAP
if (d->enable_mmap == ENABLE_MMAP_OFF) {
(void)apr_bucket_file_enable_mmap(e, 0);
}
#endif
Of course that would require to get the finfo in case we open the file
descriptor on our own.
The reading would then be something like (without error handling):
while (!APR_BRIGADE_EMPTY(bb))
{
e = APR_BUCKET_FIRST(bb)
apr_bucket_read(e, , );
apr_sha1_update_binary(, str, len);
apr_bucket_delete(e);
}
Regards
Rüdiger
Re: svn commit: r1782986 - /httpd/httpd/trunk/server/util_filter.c
On 7/5/20 9:29 AM, Christophe JAILLET wrote:
> Le 14/02/2017 à 17:43, yla...@apache.org a écrit :
>> Author: ylavic
>> Date: Tue Feb 14 16:43:25 2017
>> New Revision: 1782986
>>
>> URL:http://svn.apache.org/viewvc?rev=1782986=rev
>> Log:
>> util_filter: better ap_pass_brigade() vs empty brigades.
>>
>> Modified:
>> httpd/httpd/trunk/server/util_filter.c
>>
>> Modified: httpd/httpd/trunk/server/util_filter.c
>> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1782986=1782985=1782986=diff
>> ==
>> --- httpd/httpd/trunk/server/util_filter.c (original)
>> +++ httpd/httpd/trunk/server/util_filter.c Tue Feb 14 16:43:25 2017
>> @@ -584,9 +584,9 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
>> apr_bucket_brigade *bb)
>> {
>> if (next) {
>> - apr_bucket *e;
>> + apr_bucket *e = APR_BRIGADE_LAST(bb);
>> - if ((e = APR_BRIGADE_LAST(bb)) && APR_BUCKET_IS_EOS(e) &&
>> next->r) {
>> + if (e != APR_BRIGADE_SENTINEL(bb) && APR_BUCKET_IS_EOS(e) &&
>> next->r) {
>> /* This is only safe because HTTP_HEADER filter is always in
>> * the filter stack. This ensures that there is ALWAYS a
>> * request-based filter that we can attach this to. If the
>
> Hi,
>
> I don't really understand the change above.
>
> The commit description is clear. 'ap_pass_brigade' should deal better with
> empty brigades.
>
> However, if the last bucket is an EOS bucket, how could it be a sentinel?
> My understanding is that this change is a no-op, but I may have missed
> something.
>
> What is the rational for it?
If the brigade is empty then APR_BRIGADE_LAST(bb) points to the sentinel. Hence
The first condition of the if is not met and we
jump over the block. If the brigade is not empty we check if the last bucket
which is now not the sentinel is the eos bucket.
In the previous code the first condition in the if was always true, and I am
not sure what happened with the second condition in
case e was the sentinel.
Regards
Rüdiger
