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).
