Den mån 7 juli 2025 kl 17:15 skrev Branko Čibej <br...@apache.org>:
> On 7. 7. 25 16:07, Daniel Sahlberg wrote:
> > 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.
>
>
> The observation is correct, but changing that in only one place and then
> immediately converting it to apr_size_t is just ... I dunno, incomplete?
> I've noticed that the implementation (not just the http2 part) plays
> fast and loose with int/apr_size_t/apr_uint32_t, creating problems and
> requiring conversions. It doesn't help that APR itself is inconsistent
> about that (apr_base64, /me rolls eyes).
>
> This goes all the way to the public API in some places. Specifically,
> the source of that apr_size_t is in Serf's public bucket API, so
> non-negotiable.
>
>
> > Is there a platform where apr_size_t is < 24 bits?
>
>
> Theoretically, 16-bit platforms could have 16-bit apr_size_t, but I'm
> pretty sure we don't support those for other reasons. All platforms
> should have sizeof(apr_size_t) >= sizeof(int) but not necessarily >=
> sizeof(apr_uint32_t). I'm pretty sure APR would fall on its face on such
> platforms, so I wouldn't worry about them.
>
>
> > 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.
>
> Yes ... but we're either not compiling those bits, or they're converted
> _to_ apr_size_t. Which tends to be safe.
>
> It would be lovely to make the code consistent, but that's a *lot* of
> changes for dubious benefit (and somewhat less dubious additional source
> of bugs). I'm not in the mood to tackle that, especially given that I'm
> not the author of the code and would have to understand[1] every change
> before making it.
>
> -- Brane
>
>
> [1] Right now I'm looking at some similar warnings in the use of
> hpack_int() and, for now, I can't prove to myself that the conversions
> are safe -- so I'll leave the warnings in. I just have to find the right
> warning flags to make gcc yammer about them, too; seeing the warnings
> only on macOS isn't exactly useful, and while they're present on
> Windows, most of the MSVC warnings are red herrings and just obscure the
> real issues.
>
What I'm suggesting is to change the apr_uint32_t:s to apr_size_t. That
would match the public API and eliminate conversions.
I'm probably not experienced enough in C to see if there are potential bugs
hiding in such a change. (Of course, if there are places in the standard
where the maximum value is limited to 2^32-1 we could potentially send too
large values - but these should be taken care of anyway to avoid a
rollover, much like the existing max_payload_size check above).
Cheers,
Daniel
