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.

Reply via email to