Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 23, 2018 at 02:32:30PM +0100, Yann Ylavic wrote: > Thanks Stefan, I didn't notice before in your proposed patch, but it > looks like uint64_t casts should be apr_uint64_t too. > > Regards, > Yann. Right. I went ahead and fixed it in r1849630. Thanks, Stefan
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 23, 2018 at 10:33 AM Stefan Sperling wrote: > > On Wed, Dec 19, 2018 at 07:03:39PM +0100, Stefan Sperling wrote: > > On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote: > > > On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote: > > > > > > > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote: > > > > > But yes, upcast is better, while at it I'd go for uint64_t... > > > > > > > > Like this? > > > > > > I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;) > > > > > > Thanks for taking care of this ! > > > > Sure :) > > Committed. Thanks Stefan, I didn't notice before in your proposed patch, but it looks like uint64_t casts should be apr_uint64_t too. Regards, Yann.
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Wed, Dec 19, 2018 at 07:03:39PM +0100, Stefan Sperling wrote:
> On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> > On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote:
> > >
> > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > > But yes, upcast is better, while at it I'd go for uint64_t...
> > >
> > > Like this?
> >
> > I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
> >
> > Thanks for taking care of this !
>
> Sure :)
Committed.
> Index: modules/filters/mod_deflate.c
> ===
> --- modules/filters/mod_deflate.c (revision 1849274)
> +++ modules/filters/mod_deflate.c (working copy)
> @@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
> f->c->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
> - "Zlib: Compressed %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> r->uri);
> + "Zlib: Compressed %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> /* leave notes for logging */
> if (c->note_input_name) {
> @@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
> ctx->validation_buffer_length += valid;
>
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
> - "Zlib: Inflated %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> - r->uri);
> + "Zlib: Inflated %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
> UPDATE_CRC, ctx->proc_bb);
> @@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
> if ((ctx->stream.total_out & 0x) != compLen) {
> inflateEnd(>stream);
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
> APLOGNO(01395)
> - "Zlib: Length %ld of inflated data
> does "
> - "not match expected value %ld",
> - ctx->stream.total_out, compLen);
> + "Zlib: Length %" APR_UINT64_T_FMT
> + " of inflated data does not match"
> + " expected value %ld",
> + (uint64_t)ctx->stream.total_out,
> compLen);
> return APR_EGENERAL;
> }
> }
> @@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
> */
> flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
> - "Zlib: Inflated %ld to %ld : URL %s",
> - ctx->stream.total_in, ctx->stream.total_out,
> r->uri);
> + "Zlib: Inflated %" APR_UINT64_T_FMT
> + " to %" APR_UINT64_T_FMT " : URL %s",
> + (uint64_t)ctx->stream.total_in,
> + (uint64_t)ctx->stream.total_out, r->uri);
>
> if (ctx->validation_buffer_length == VALIDATION_SIZE) {
> unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Wed, Dec 19, 2018 at 02:58:28PM +0100, Yann Ylavic wrote:
> On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote:
> >
> > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> > > But yes, upcast is better, while at it I'd go for uint64_t...
> >
> > Like this?
>
> I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;)
>
> Thanks for taking care of this !
Sure :)
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849274)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,8 +893,10 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Compressed %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
/* leave notes for logging */
if (c->note_input_name) {
@@ -1438,9 +1440,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out,
- r->uri);
+ "Zlib: Inflated %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1462,10 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
- "not match expected value %ld",
- ctx->stream.total_out, compLen);
+ "Zlib: Length %" APR_UINT64_T_FMT
+ " of inflated data does not match"
+ " expected value %ld",
+ (uint64_t)ctx->stream.total_out,
compLen);
return APR_EGENERAL;
}
}
@@ -1628,8 +1632,10 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Inflated %" APR_UINT64_T_FMT
+ " to %" APR_UINT64_T_FMT " : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Wed, Dec 19, 2018 at 9:53 AM Stefan Sperling wrote: > > On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote: > > But yes, upcast is better, while at it I'd go for uint64_t... > > Like this? I think APR_UINT64_T_FMT/apr_uint64_t would be more portable ;) Thanks for taking care of this ! Regards, Yann.
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Tue, Dec 18, 2018 at 12:29:18AM +0100, Yann Ylavic wrote:
> But yes, upcast is better, while at it I'd go for uint64_t...
Like this?
I've noticed that the same problem seems to exist in some other modules.
I'll send separate patches for those once this patch has settled.
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849274)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,8 +893,9 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Compressed %llu to %llu: URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
/* leave notes for logging */
if (c->note_input_name) {
@@ -1438,9 +1439,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out,
- r->uri);
+ "Zlib: Inflated %llu to %llu : URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
consume_buffer(ctx, c, c->bufferSize - ctx->stream.avail_out,
UPDATE_CRC, ctx->proc_bb);
@@ -1459,9 +1460,9 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
+ "Zlib: Length %llu of inflated data does
"
"not match expected value %ld",
- ctx->stream.total_out, compLen);
+ (uint64_t)ctx->stream.total_out,
compLen);
return APR_EGENERAL;
}
}
@@ -1628,8 +1629,9 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
- ctx->stream.total_in, ctx->stream.total_out, r->uri);
+ "Zlib: Inflated %llu to %llu: URL %s",
+ (uint64_t)ctx->stream.total_in,
+ (uint64_t)ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
unsigned long compCRC, compLen;
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Mon, Dec 17, 2018 at 5:40 PM William A Rowe Jr wrote: > > On Sun, Dec 16, 2018 at 7:27 AM Yann Ylavic wrote: >> >> >> Since it's logging only, it may be easier to cast to (long) each >> total_in/out though. > > Downcast? Why not upcast to apr_off_t and use the _FMT macro as first > suggested? Neither is ideal I think since in the unsigned long case that's still a cast to signed off_t. But yes, upcast is better, while at it I'd go for uint64_t...
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 7:27 AM Yann Ylavic wrote: > > Since it's logging only, it may be easier to cast to (long) each > total_in/out though. > Downcast? Why not upcast to apr_off_t and use the _FMT macro as first suggested?
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 2:21 PM Yann Ylavic wrote:
>
> On Sun, Dec 16, 2018 at 2:14 PM Stefan Sperling wrote:
> >
> > The file /usr/include/zlib.h I have on OpenBSD -current has this:
> >
> > typedef struct z_stream_s {
> > [...]
> > z_off_t total_in; /* total nb of input bytes read so far */
> > [...]
> > z_off_t total_out; /* total nb of bytes output so far */
> > [...]
> > } z_stream;
>
> Ouch.
>
> >
> > This looks like an OpenBSD-specific change though (diff below).
> > I guess this will force OpenBSD to carry a local patch for
> > mod_deflate then, or just compile without -Werror.
>
> I think we need a bit of #ifdef-ery in mod_deflate then, a BSD only
> fix wouldn't help those who build from sources.
>
> Do you know which BSD(s) are concerned (if not all)?
Since it's logging only, it may be easier to cast to (long) each
total_in/out though.
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 2:14 PM Stefan Sperling wrote:
>
> The file /usr/include/zlib.h I have on OpenBSD -current has this:
>
> typedef struct z_stream_s {
> [...]
> z_off_t total_in; /* total nb of input bytes read so far */
> [...]
> z_off_t total_out; /* total nb of bytes output so far */
> [...]
> } z_stream;
Ouch.
>
> This looks like an OpenBSD-specific change though (diff below).
> I guess this will force OpenBSD to carry a local patch for
> mod_deflate then, or just compile without -Werror.
I think we need a bit of #ifdef-ery in mod_deflate then, a BSD only
fix wouldn't help those who build from sources.
Do you know which BSD(s) are concerned (if not all)?
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 02:03:45PM +0100, Yann Ylavic wrote:
> On Sun, Dec 16, 2018 at 1:28 PM Stefan Sperling wrote:
> >
> > mod_deflates hard-codes some off_t format directives to "%ld".
> > It seems to me this code should use the macro provided by APR instead.
> >
> > Looking for another pair of eyes. Does this patch look good to commit?
>
> It seems that zlib defines total_in/out as uLong, i.e.:
> typedef struct z_stream_s {
> [...]
> uLongtotal_in; /* total number of input bytes read so far */
> [...]
> uLongtotal_out; /* total number of bytes output so far */
> [...]
> } z_stream;
>
> So %ld (or %lu) looks correct to me.
>
> (mod_brotli uses apr_off_t for them but it's an internal struct there).
>
> Regards,
> Yann.
Thanks for checking. This seems to depend on the version of zlib.
The file /usr/include/zlib.h I have on OpenBSD -current has this:
typedef struct z_stream_s {
[...]
z_off_t total_in; /* total nb of input bytes read so far */
[...]
z_off_t total_out; /* total nb of bytes output so far */
[...]
} z_stream;
And the compiler (clang) now rightly complains as follows:
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:27: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:856:49: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:31: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out,
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1423:53: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out,
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1450:39: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_out, compLen);
^
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:27: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^~~~
/home/stsp/svn/src/httpd-2.4.37/modules/filters/mod_deflate.c:1626:49: error:
format specifies type 'long' but the argument has type 'off_t'
(aka 'long long') [-Werror,-Wformat]
ctx->stream.total_in, ctx->stream.total_out, r->uri);
^
7 errors generated.
This looks like an OpenBSD-specific change though (diff below).
I guess this will force OpenBSD to carry a local patch for
mod_deflate then, or just compile without -Werror.
RCS file: /cvs/src/lib/libz/zlib.h,v
Working file: zlib.h
head: 1.10
[...]
revision 1.7
date: 2003/12/16 22:35:50; author: henning; state: Exp; lines: +2 -2;
total_in and total_out need to be off_t, not unsigned long.
some bugs return: i fixed the same some months ago when we had this other gzip
there.
this bug resulted in wrong size stats for > 4GB files, and in the case
that the input file was > 4GB and could be compressed to < 4GB gzip
not zipping it as it would grow in its eyes.
=
Index: zlib.h
===
RCS file: /cvs/src/lib/libz/zlib.h,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -p -r1.6 -r1.7
--- zlib.h 16 Dec 2003 22:33:02 -
Re: [PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
On Sun, Dec 16, 2018 at 1:28 PM Stefan Sperling wrote:
>
> mod_deflates hard-codes some off_t format directives to "%ld".
> It seems to me this code should use the macro provided by APR instead.
>
> Looking for another pair of eyes. Does this patch look good to commit?
It seems that zlib defines total_in/out as uLong, i.e.:
typedef struct z_stream_s {
[...]
uLongtotal_in; /* total number of input bytes read so far */
[...]
uLongtotal_out; /* total number of bytes output so far */
[...]
} z_stream;
So %ld (or %lu) looks correct to me.
(mod_brotli uses apr_off_t for them but it's an internal struct there).
Regards,
Yann.
[PATCH] mod_deflate: hardcoded "%ld" -> APR_OFF_T_FMT
mod_deflates hard-codes some off_t format directives to "%ld".
It seems to me this code should use the macro provided by APR instead.
Looking for another pair of eyes. Does this patch look good to commit?
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1849023)
+++ modules/filters/mod_deflate.c (working copy)
@@ -893,7 +893,8 @@ static apr_status_t deflate_out_filter(ap_filter_t
f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(ctx->bb, b);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01384)
- "Zlib: Compressed %ld to %ld : URL %s",
+ "Zlib: Compressed %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out, r->uri);
/* leave notes for logging */
@@ -1438,7 +1439,8 @@ static apr_status_t deflate_in_filter(ap_filter_t
ctx->validation_buffer_length += valid;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01393)
- "Zlib: Inflated %ld to %ld : URL %s",
+ "Zlib: Inflated %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out,
r->uri);
@@ -1459,7 +1461,8 @@ static apr_status_t deflate_in_filter(ap_filter_t
if ((ctx->stream.total_out & 0x) != compLen) {
inflateEnd(>stream);
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
APLOGNO(01395)
- "Zlib: Length %ld of inflated data does "
+ "Zlib: Length %" APR_OFF_T_FMT
+ " of inflated data does "
"not match expected value %ld",
ctx->stream.total_out, compLen);
return APR_EGENERAL;
@@ -1628,7 +1631,8 @@ static apr_status_t inflate_out_filter(ap_filter_t
*/
flush_libz_buffer(ctx, c, inflate, Z_SYNC_FLUSH, UPDATE_CRC);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01398)
- "Zlib: Inflated %ld to %ld : URL %s",
+ "Zlib: Inflated %" APR_OFF_T_FMT
+ " to %" APR_OFF_T_FMT " : URL %s",
ctx->stream.total_in, ctx->stream.total_out, r->uri);
if (ctx->validation_buffer_length == VALIDATION_SIZE) {
