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.


Any better ideas?

-- Brane

Reply via email to