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.
