gh-ARMmbed/mbed-crypto

Watch

Fork 0

Fix warning with gcc7 - maximum allocation exceeded #377

Merged

simonbutcher merged 1 commit from gcc7-warning-fix into development

2020-03-17 13:44:24 -05:00

simonbutcher commented

2020-02-26 11:31:14 -06:00

(Migrated from github.com)

Adds additional limits checks on the ASSERT_ALLOC macros in the test suite helper functions to remove the warning of an allocation potentially exceeding the maximum object size. eg -Werror=alloc-size-larger-than.

The warning is present in GCC 7, and is the reason that the test_m32_everest component in all.sh is failing with the ubuntu-18.04 docker image.

Long explanation of the fix

In the test suite test_suite_psa there's a test case function of generate_random() which takes int as a parameter bytes_arg and assigns it to bytes as a size_t. The parameter comes from the data file so comes in via standard IO so could be at the limit. Within this function, it makes a call to the macro ASSERT_ALLOC(), passing that parameter bytes + sizeof(trail), without any boundary checks on bytes unchecked.

Within ASSERT_ALLOC there is then a call to calloc that takes a parameter of size as size_t, and passes bytes_arg an int into it.

In the Everest m32 test, this is failing with the warning? Why? Well, we've moved from LP64, where int is 32bits, and size_t is 64 bits, to ILP32 where int is 32bits and size_t is 32 bits

In LP64, an int (32 bits) plus sizeof(trail) (constant, small), will always be less than the limit of a size_t (64 bits), whilst in ILP32, it could exceed the size of the size_t (32 bits).

And so we get the warning.

Adds additional limits checks on the `ASSERT_ALLOC` macros in the test suite helper functions to remove the warning of an allocation potentially exceeding the maximum object size. eg `-Werror=alloc-size-larger-than`. The warning is present in GCC 7, and is the reason that the `test_m32_everest` component in `all.sh` is failing with the `ubuntu-18.04` docker image. ### Long explanation of the fix In the test suite `test_suite_psa` there's a test case function of `generate_random()` which takes `int` as a parameter `bytes_arg` and assigns it to `bytes` as a `size_t`. The parameter comes from the data file so comes in via standard IO so could be at the limit. Within this function, it makes a call to the macro `ASSERT_ALLOC()`, passing that parameter `bytes` + `sizeof(trail)`, without any boundary checks on `bytes` unchecked. Within `ASSERT_ALLOC` there is then a call to `calloc` that takes a parameter of size as `size_t`, and passes `bytes_arg` an `int` into it. In the Everest m32 test, this is failing with the warning? Why? Well, we've moved from LP64, where `int` is 32bits, and `size_t` is 64 bits, to ILP32 where `int` is 32bits and `size_t` is 32 bits In LP64, an `int` (32 bits) plus `sizeof(trail)` (constant, small), will always be less than the limit of a `size_t` (64 bits), whilst in ILP32, it could exceed the size of the `size_t` (32 bits). And so we get the warning.

gilles-peskine-arm (Migrated from github.com) requested changes

2020-02-26 12:17:41 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

I don't understand this change. It doesn't seem right and I don't know what it is supposed to fix.

tests/suites/helpers.function

I don't understand this change. Why would ASSERT_ALLOC to transform a request to allocate SIZE_MAX bytes into successfully doing nothing? Why would ASSERT_ALLOC care about a request to allocate SIZE_MAX bytes? Why wouldn't a request to allocate SIZE_MAX bytes a potential bug of the caller?

I don't understand this change. Why would `ASSERT_ALLOC` to transform a request to allocate `SIZE_MAX` bytes into successfully doing nothing? Why would `ASSERT_ALLOC` care about a request to allocate `SIZE_MAX` bytes? Why wouldn't a request to allocate `SIZE_MAX` bytes a potential bug of the caller?

tests/suites/helpers.function

Preexisting: for some reason, probably a bad merge or side-port from mbedtls to mbed-crypto, ASSERT_ALLOC is defined twice. If you need to change the macro, please remove one occurrence. Choose which one to make the crypto and tls copies of the file as close as possible.

Preexisting: for some reason, probably a bad merge or side-port from mbedtls to mbed-crypto, `ASSERT_ALLOC` is defined twice. If you need to change the macro, please remove one occurrence. Choose which one to make the crypto and tls copies of the file as close as possible.

simonbutcher (Migrated from github.com) reviewed

2020-02-27 06:09:01 -06:00

tests/suites/helpers.function

I've described the issue above. The issue is actually in generate_random(). I could have put a bounds check there, and in hindsight, at the point we receive IO it should always be checked anyway, but I put it before each calloc, which is more conservative, and because I was concerned we may get more warnings.

I've described the issue above. The issue is actually in `generate_random()`. I could have put a bounds check there, and in hindsight, at the point we receive IO it should always be checked anyway, but I put it before each `calloc`, which is more conservative, and because I was concerned we may get more warnings.

simonbutcher (Migrated from github.com) reviewed

2020-02-27 06:38:20 -06:00

tests/suites/helpers.function

Fixed.

gilles-peskine-arm commented

2020-03-02 05:48:34 -06:00

(Migrated from github.com)

Steps to reproduce: the warning appears with GCC 7.4 (but not with 6.4 or 5.4), when compiling for a 32-bit platform such as x86, only with -O2 (not with -O1 or -Os), only when UBSan is enabled (-fsanitized=undefined). It appears only in one place in the code, in test_suite_psa_crypto.function, in generate_random.

void generate_random( int bytes_arg )
{
    size_t bytes = bytes_arg;
    const unsigned char trail[] = "don't overwrite me";
    unsigned char *output = NULL;
    unsigned char *changed = NULL;
    size_t i;
    unsigned run;

    ASSERT_ALLOC( output, bytes + sizeof( trail ) );
    ASSERT_ALLOC( changed, bytes );

The warning is about the last line quoted above.

suites/test_suite_psa_crypto.function: In function 'test_generate_random':
suites/helpers.function:398:25: error: argument 2 value '4294967277' exceeds maximum object size 2147483647 [-Werror=alloc-size-larger-than=]
             ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \
             ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                           ( length ) );           \
                                           ~~~~~~~~~~~~
suites/test_suite_psa_crypto.function:5301:5: note: in expansion of macro 'ASSERT_ALLOC'
In file included from suites/helpers.function:5:0:
/usr/include/stdlib.h:541:14: note: in a call to allocation function 'calloc' declared here
 extern void *calloc (size_t __nmemb, size_t __size)
              ^~~~~~

4294967277 = 2^32 - 19 = 2^32 - sizeof(trail). Since the warning is only triggered at a high optimization level, I suspect it comes from a pass that's done after inlining, and that it's specific to the branching paths through the conditional statements inside the ASSERT_ALLOC and inside the UBSan calloc wrapper which tests if the first allocation is 0 bytes. On an architecture where size_t has the same size as unsigned int, this happens if bytes_arg == -sizeof(trail). On an architecture where size_t is larger, this never happens.

Based on this analysis my proposed fix is to add

    TEST_ASSERT( bytes_arg >= 0 );

before the ASSERT_ALLOC calls in generate_random. I've confirmed locally that this makes the build pass with gcc-7.4 -m32 -O2 -fsanitize=undefined.

I'm open to other approaches. But I still don't see a good argument for “fixing” ASSERT_ALLOC. The current behavior is the desired behavior. If some test code is calling it with arguments that are out of range, that's a bug in the test code and we should not silence that warning.

Steps to reproduce: the warning appears with GCC 7.4 (but not with 6.4 or 5.4), when compiling for a 32-bit platform such as x86, only with `-O2` (not with `-O1` or `-Os`), only when UBSan is enabled (`-fsanitized=undefined`). It appears only in one place in the code, in `test_suite_psa_crypto.function`, in `generate_random`. ``` void generate_random( int bytes_arg ) { size_t bytes = bytes_arg; const unsigned char trail[] = "don't overwrite me"; unsigned char *output = NULL; unsigned char *changed = NULL; size_t i; unsigned run; ASSERT_ALLOC( output, bytes + sizeof( trail ) ); ASSERT_ALLOC( changed, bytes ); ``` The warning is about the last line quoted above. ``` suites/test_suite_psa_crypto.function: In function 'test_generate_random': suites/helpers.function:398:25: error: argument 2 value '4294967277' exceeds maximum object size 2147483647 [-Werror=alloc-size-larger-than=] ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \ ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ( length ) ); \ ~~~~~~~~~~~~ suites/test_suite_psa_crypto.function:5301:5: note: in expansion of macro 'ASSERT_ALLOC' In file included from suites/helpers.function:5:0: /usr/include/stdlib.h:541:14: note: in a call to allocation function 'calloc' declared here extern void *calloc (size_t __nmemb, size_t __size) ^~~~~~ ``` 4294967277 = 2^32 - 19 = 2^32 - `sizeof(trail)`. Since the warning is only triggered at a high optimization level, I suspect it comes from a pass that's done after inlining, and that it's specific to the branching paths through the conditional statements inside the `ASSERT_ALLOC` and inside the UBSan `calloc` wrapper which tests if the first allocation is 0 bytes. On an architecture where `size_t` has the same size as `unsigned int`, this happens if `bytes_arg == -sizeof(trail)`. On an architecture where `size_t` is larger, this never happens. Based on this analysis my proposed fix is to add ``` TEST_ASSERT( bytes_arg >= 0 ); ``` before the `ASSERT_ALLOC` calls in `generate_random`. I've confirmed locally that this makes the build pass with `gcc-7.4 -m32 -O2 -fsanitize=undefined`. I'm open to other approaches. But I still don't see a good argument for “fixing” `ASSERT_ALLOC`. The current behavior is the desired behavior. If some test code is calling it with arguments that are out of range, that's a bug in the test code and we should not silence that warning.

gilles-peskine-arm (Migrated from github.com) requested changes

2020-03-02 05:49:22 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

I don't think this is the right approach (see my analysis in https://github.com/ARMmbed/mbed-crypto/pull/377#issuecomment-593364500).
Even if it is, the proposed fix is wrong.

1. I don't think this is the right approach (see my analysis in https://github.com/ARMmbed/mbed-crypto/pull/377#issuecomment-593364500). 2. Even if it is, the proposed fix is wrong.

tests/suites/helpers.function

Regardless of whether this is the right place for the fix, the behavior is not correct. If length == SIZE_MAX, ASSERT_ALLOC should mark the test case as failed and ASSERT_ALLOC_WEAK should mark the test case as skipped.

Regardless of whether this is the right place for the fix, the behavior is not correct. If `length == SIZE_MAX`, `ASSERT_ALLOC` should mark the test case as failed and `ASSERT_ALLOC_WEAK` should mark the test case as skipped.

This may pacify GCC 7 but it angers GCC 5.

suites/helpers.function:346:43: warning: comparison is always true due to limited range of data type [-Wtype-limits]

         if( ( length ) != 0 && ( length ) < SIZE_MAX )            \

                                           ^

suites/test_suite_psa_crypto.function:5243:5: note: in expansion of macro 'ASSERT_ALLOC'

     ASSERT_ALLOC( actual_output, MAX( expected_output1->len,

     ^

This may pacify GCC 7 but it angers GCC 5. ``` suites/helpers.function:346:43: warning: comparison is always true due to limited range of data type [-Wtype-limits] if( ( length ) != 0 && ( length ) < SIZE_MAX ) \ ^ suites/test_suite_psa_crypto.function:5243:5: note: in expansion of macro 'ASSERT_ALLOC' ASSERT_ALLOC( actual_output, MAX( expected_output1->len, ^ ```

simonbutcher commented

2020-03-02 06:13:51 -06:00

(Migrated from github.com)

However - I will say - one of my many concerns with the Docker files (and therefore the CI) is that the GCC version is wrong. We shouldn't care about 5.4, it's ancient and the branch is not maintained. We should be testing with GCC 7 or 8 (as does Mbed OS).

This fix is part of the reason why we can't yet make that jump.

@gilles-peskine-arm - I think your proposal is the better fix, so I'll swap to that. I didn't give this too much thought as it's test code and I just wanted to be rid of yet another distraction, but I think it's better to test the input than add a check at the point of failure. However - I will say - one of my many concerns with the Docker files (and therefore the CI) is that the GCC version is wrong. We shouldn't care about 5.4, it's ancient and [the branch is not maintained](https://gcc.gnu.org/develop.html#timeline). We should be testing with GCC 7 or 8 (as does Mbed OS). This fix is part of the reason why we can't yet make that jump.

gilles-peskine-arm commented

2020-03-02 06:20:14 -06:00

(Migrated from github.com)

On the topic of GCC versions, I think we should actually test with the oldest version we're willing to support (which is definitely ≤5.4 since that's the version on Ubuntu 16.04 which we still use actively) and with the latest release. But that's a topic for another day.

simonbutcher commented

2020-03-03 11:29:54 -06:00

(Migrated from github.com)

I've reworked the PR to adopt @gilles-peskine-arm's simpler fix.

If you could review again, thanks.

I've reworked the PR to adopt @gilles-peskine-arm's simpler fix. If you could review again, thanks.

gilles-peskine-arm (Migrated from github.com) approved these changes

2020-03-04 07:31:22 -06:00

simonbutcher commented

2020-03-04 10:40:49 -06:00

(Migrated from github.com)

Raised as bug #385 for tracking. (Which I should have done when I first created this PR).

paul-elliott-arm (Migrated from github.com) approved these changes

2020-03-16 14:28:30 -05:00

Patater (Migrated from github.com) approved these changes

2020-03-17 04:35:15 -05:00

Patater (Migrated from github.com) left a comment

LGTM

Patater commented

2020-03-17 04:36:30 -05:00

(Migrated from github.com)

No backports required, as this is for PSA code.

Patater commented

2020-03-17 04:50:04 -05:00

(Migrated from github.com)

Restarted merge job, as Mbed OS importers should not be looking for apache-2.0.txt anymore.

Restarted merge job, as Mbed OS importers should not be looking for `apache-2.0.txt` anymore.

Patater commented

2020-03-17 13:44:16 -05:00

(Migrated from github.com)

Merge job failed on Mbed OS with the attestation service not being able to build due to API changes in PSA Crypto. Non-blocking issue unrelated to this PR. Merging.