On 7. 7. 25 11:08, Branko Čibej wrote:
As you've probably noticed, I've been going through compilation
warnings, especially implicit narrowing conversions, and trying to fix
them in a sane way. Many of them, by the way, I don't see with gcc
because it doesn't have a -Wshorten-64-to-32 flag, so I'm using clang.
This time I found a gem:
.../protocols/http2_stream.c:162:48: warning: implicit conversion
loses integer precision:
'apr_size_t' (aka 'unsigned long') to 'apr_uint32_t' (aka
'unsigned int') [-Wshorten-64-to-32]
146 | max_payload_size,
| ^~~~~~~~~~~~~~~~
This is in a call to serf__bucket_http2_frame_create(), which expects
max_payload_size as an apr_uint32_t.
But then it goes on to allocate a serf_http2_frame_context_t (see:
http2_frame_buckets.h line 603) which has a member max_payload_size
that is an apr_size_t. And if that's not funny enough, here's how the
struct initialisation actually happens:
serf__bucket_http2_frame_create(serf_bucket_t *stream,
unsigned char frame_type,
unsigned char flags,
apr_int32_t *stream_id,
void(*stream_id_alloc)(
void *baton,
apr_int32_t *stream_id),
void *stream_id_baton,
apr_uint32_t max_payload_size,
serf_bucket_alloc_t *alloc)
{
serf_http2_frame_context_t *ctx = serf_bucket_mem_alloc(alloc,
sizeof(*ctx));
ctx->alloc = alloc;
ctx->stream = stream;
ctx->chunk = serf_bucket_aggregate_create(alloc);
ctx->max_payload_size = max_payload_size;
ctx->frametype = frame_type;
ctx->flags = flags;
if (max_payload_size > 0xFFFFFF)
max_payload_size = 0xFFFFFF;
...
From this point onward, max_payload_size (the function argument, an
apr_uint32_t) is not used. Um. The size adjustment is a no-op?
So to silence the warning, it would be enough to change the type of
the function argument. But this bit of code is actually nonsense.
Should the max size be limited to 16 MiB or not? If yes, and it isn't,
how does the HTTP/2 code even work correctly? Or maybe we just haven't
encountered a case where it breaks?
I don't know anything about HTTP/2 framing. RFC 9113/4.2 says that 16
MiB is the upper limit. My gut feeling is to move the size adjustment
before 'ctx' initialisation, and to change the type of the parameter
-- the latter because Serf operates with apr_size_t all the time.
Specifically, this silences the warning and all tests still pass,
including a checkout with Subversion over https (though I've no idea how
to verify if that actually uses HTTP/2; any pointers would be welcome).
Index: buckets/http2_frame_buckets.c
===================================================================
--- buckets/http2_frame_buckets.c (revision 1927021)
+++ buckets/http2_frame_buckets.c (working copy)
@@ -630,12 +630,17 @@
void *baton,
apr_int32_t *stream_id),
void *stream_id_baton,
- apr_uint32_t max_payload_size,
+ apr_size_t max_payload_size,
serf_bucket_alloc_t *alloc)
{
serf_http2_frame_context_t *ctx = serf_bucket_mem_alloc(alloc,
sizeof(*ctx));
+ /* The upper limit for HTTP/2 MAX_FRAME_SIZE is 16 M1B.
+https://www.rfc-editor.org/rfc/rfc9113.html#section-4.2 */
+ if (max_payload_size > 0xFFFFFF)
+ max_payload_size = 0xFFFFFF;
+
ctx->alloc = alloc;
ctx->stream = stream;
ctx->chunk = serf_bucket_aggregate_create(alloc);
@@ -643,9 +648,6 @@
ctx->frametype = frame_type;
ctx->flags = flags;
- if (max_payload_size > 0xFFFFFF)
- max_payload_size = 0xFFFFFF;
-
if (!stream_id_alloc || (stream_id && *stream_id >= 0))
{
/* Avoid all alloc handling; we know the final id */
@@ -980,4 +982,3 @@
serf_http2_frame_get_remaining,
serf_http2_frame_set_config
};
-
Index: protocols/http2_buckets.h
===================================================================
--- protocols/http2_buckets.h (revision 1927021)
+++ protocols/http2_buckets.h (working copy)
@@ -202,7 +202,7 @@
void *baton,
apr_int32_t *stream_id),
void *stream_id_baton,
- apr_uint32_t max_payload_size,
+ apr_size_t max_payload_size,
serf_bucket_alloc_t *alloc);
/* ==================================================================== */
Index: buckets/http2_frame_buckets.c
===================================================================
--- buckets/http2_frame_buckets.c (revision 1927021)
+++ buckets/http2_frame_buckets.c (working copy)
@@ -630,12 +630,17 @@
void *baton,
apr_int32_t *stream_id),
void *stream_id_baton,
- apr_uint32_t max_payload_size,
+ apr_size_t max_payload_size,
serf_bucket_alloc_t *alloc)
{
serf_http2_frame_context_t *ctx = serf_bucket_mem_alloc(alloc,
sizeof(*ctx));
+ /* The upper limit for HTTP/2 MAX_FRAME_SIZE is 16 M1B.
+ https://www.rfc-editor.org/rfc/rfc9113.html#section-4.2 */
+ if (max_payload_size > 0xFFFFFF)
+ max_payload_size = 0xFFFFFF;
+
ctx->alloc = alloc;
ctx->stream = stream;
ctx->chunk = serf_bucket_aggregate_create(alloc);
@@ -643,9 +648,6 @@
ctx->frametype = frame_type;
ctx->flags = flags;
- if (max_payload_size > 0xFFFFFF)
- max_payload_size = 0xFFFFFF;
-
if (!stream_id_alloc || (stream_id && *stream_id >= 0))
{
/* Avoid all alloc handling; we know the final id */
@@ -980,4 +982,3 @@
serf_http2_frame_get_remaining,
serf_http2_frame_set_config
};
-
Index: protocols/http2_buckets.h
===================================================================
--- protocols/http2_buckets.h (revision 1927021)
+++ protocols/http2_buckets.h (working copy)
@@ -202,7 +202,7 @@
void *baton,
apr_int32_t *stream_id),
void *stream_id_baton,
- apr_uint32_t max_payload_size,
+ apr_size_t max_payload_size,
serf_bucket_alloc_t *alloc);
/* ==================================================================== */
