Den mån 7 juli 2025 kl 21:43 skrev Branko Čibej <br...@apache.org>:
> On 7. 7. 25 21:30, Daniel Sahlberg wrote:
> > 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).
>
> By "the standard", I expect you mean the HTTP(/2) RFCs? I haven't read
> them all, but surely there are some "SHOULD" and "MUST" kinds of limits.
> At least the MAX_FRAME_SIZE has both a lower and an upper bound defined,
> but I don't know whether the actual value is controlled by the server or
> somehow negotiated between server and client.
>
Yes, I meant the RFCs.
In the http2_protocol_t struct, for each setting always two there are, no
more, no less. An "lr" (local->remote) and a "rl" (remote->local), each
controlled by either the server or the client.
>
> About all those apr_uint32_t's ... I'm not seeing any warnings from
> there. But, again, I haven't investigated.
>
For the lr_max_framesize, there is code to parse it from the SETTIGS frame
(ie, sent from the server), this code seems to read the binary data to an
apr_uint32_t so obviously no warning there.
rl_max_framesize is set to a default value (16384) and there is no way to
change it. There should probably be a public API to set it, in which case
we'd have to decide if we should use apr_size_t or apr_uint32_t. In either
case we'd have to check the min and max value according to the bounds in
the RFC.
Cheers,
Daniel
