libs3: Build fails with format-overflow warning on almalinux:9

[alanking]

This warning was added in gcc 7 at the latest and yet I have not seen it trip on any platforms until now.

I'm using gcc-toolset-12 because gcc-toolset-11 is not available in repos currently configured.

From the log...

INFO -   stderr: src/s3.c: In function ‘listBucketCallback’:
src/s3.c:1230:35: error: ‘%4llu’ directive writing between 4 and 17 bytes into a region of size 16 [-Werror=format-overflow=]
 1230 |                 sprintf(sizebuf, "%4lluK",
      |                                   ^~~~~
src/s3.c:1230:34: note: directive argument in the range [0, 18014398509481983]
 1230 |                 sprintf(sizebuf, "%4lluK",
      |                                  ^~~~~~~~
src/s3.c:1230:17: note: ‘sprintf’ output between 6 and 19 bytes into a destination of size 16
 1230 |                 sprintf(sizebuf, "%4lluK",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~
 1231 |                         ((unsigned long long) content->size) / 1024ULL);
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [GNUmakefile:222: build/obj/s3.o] Error 1
make[1]: *** Waiting for unfinished jobs....
INFO -          
ERROR - build failed
Building [libs3]

[alanking]

This may warrant an issue in the libs3 repo, but figured I'd start here.

[korydraughn]

It appears that gcc is detecting potential overflow, so I think you're correct that an issue should be created.

At least so we're aware of it.

[alanking]

Okay, I created irods/libs3#26 to track the issue in the libs3 repo. We can determine here whether we can ignore this warning for our purposes and use the other issue to address the problem.

[alanking]

I took another look at this...

            char sizebuf[16];
            if (content->size < 100000) {
                sprintf(sizebuf, "%5llu", (unsigned long long) content->size);
            }
            else if (content->size < (1024 * 1024)) {
                sprintf(sizebuf, "%4lluK",
                        ((unsigned long long) content->size) / 1024ULL);
            }

In order to execute this code path, content->size must be less than 1024 * 1024 = 1048576. The maximum value, then, is 1048575. The value being printed into the string in this case would be 1048575 / 1024 = 1023.

sizebuf is a char[16] which is sufficient to hold the 4 digits, plus 'K', plus null terminator - a total of 6 bytes at maximum.

According to GNU's documentation on the formatted output functions...

Warning: The sprintf function can be dangerous because it can potentially output more characters than can fit in the allocation size of the string s. Remember that the field width given in a conversion specification is only a minimum value.

However...

A null character is written to mark the end of the string.

Seeing as the string will never exceed 5 characters (even in the event of an underflow), we know that the buffer will never overflow in this case.

Does this assessment seem correct? I think this is safe to ignore.

[korydraughn]

Maybe.

Does the warning appear if you use sprintf correctly?
Does the warning appear if you use snprintf?
What happens if content->size is -1? (Assuming it is an int)

This has likely existed for several years now. And given there hasn't been any updates to the upstream repo, it is likely safe to say we own it now. Meaning we can fix the warning. The question then becomes, how do we test it?

[alanking]

Does the warning appear if you use sprintf correctly?

Sorry, I don't understand. Is sprintf being used incorrectly?

Does the warning appear if you use snprintf?

I would think not. That would be my suggested solution to irods/libs3#26.

What happens if content->size is -1? (Assuming it is an int)

content->size is not an int, it is uint64_t. An underflow results in a positive value that is larger than 1024 * 1024.

[korydraughn]

Sorry, I don't understand. Is sprintf being used incorrectly?

I don't think it is being used incorrectly, but perhaps increasing the size of sizebuf resolves the warning. We give sprintf a buffer of size 16, and the compiler seems to be warning us that the buffer is too small to hold the full range of the type.

content->size is not an int, it is uint64_t. An underflow results in a positive value that is larger than 1024 * 1024.

Good.

[alanking]

I don't think it is being used incorrectly, but perhaps increasing the size of sizebuf resolves the warning. We give sprintf a buffer of size 16, and the compiler seems to be warning us that the buffer is too small to hold the full range of the type.

Fair enough. Can try that as well.

[alanking]

irods/libs3#26 is now resolved. This can be resolved by bumping the sha.

[korydraughn]

Let's go ahead and take care of that.