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.