Den mån 7 juli 2025 kl 12:08 skrev Branko Čibej <br...@apache.org>:
> 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.
>
> Not sure how much I can help out here and I'm sure you read the logs as
well as I do, but it seems there are only a few revisions of that
particular part of the code. Initially, it seems max_payload_size was
indeed apr_size_t but r1712557 changed to the current apr_uint32_t: "
(serf__bucket_http2_frame_create): Remove some arguments. Limits
max_payload_size to a uint32, as that is how http2 configures it and it
has to fit in 24 bits anyway."
In the case you quoted above, stream_send_headers(), the max_payload_size
is apr_size_t. Checking some places this is called find
http2_stream_enque_response which gets the argument from
serf_http2__max_payload_size which return apr_size_t BUT it get the actual
returned number from an serf_http2_protocol_t struct member
lr_max_framesize which in turn is (*TADA*) apr_uint32_t.
It seems Bert's observation "it has to fit in 24 bits anyway" is correct
and from that point of view it makes sense to use apr_uint32_t. But I don't
see any real reason why it would be unreasonable to use apr_size_t -
personally I think that type is more logical. Is there a platform where
apr_size_t is < 24 bits?
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;
> +
>
>
+1 to moving.
> 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);
>
> /* ==================================================================== */
>
>
Makes sense to me, but it would probably make sense to update the other
instances where we work on the size - as mentioned above the
serf_http2_protocol_t is riddled with apr_uint32_t:s.
