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.
