Fix signed-to-unsigned integer conversion warning in X.509 module

[hanno-becker]

Fixes #2212.

[hanno-becker]

@sbutcher-arm CI failure due to known fragmentation bug in old version of GnuTLS.

[irwir]

If the intention was to get all flag bits set, then it would be better to use zero complement.

ver_chain->items[i].flags = ~0;

Another possibility might be UINT32_MAX constant.

[simonbutcher]

This is a trivial fix, but if it has a Github issue, it should really have a ChangeLog entry.

[hanno-becker]

@sbutcher-arm Ok.

[simonbutcher]

This breaks the buildbot CI on the Visual Studio build. (See: 'any - polarssl_win32 (msvc12_64_make) Build #126'). Log extract:

Microsoft (R) Build Engine version 12.0.31101.0
[Microsoft .NET Framework, version 4.0.30319.18444]
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 1/8/2019 4:08:17 PM.
Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\ALL_BUILD.vcxproj" on node 1 (default targets).
Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\ALL_BUILD.vcxproj" (1) is building "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\ZERO_CHECK.vcxproj" (2) on node 1 (default targets).
......
  x509_crt.c
C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\x509_crt.c(371): error C2220: warning treated as error - no 'object' file generated [C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\mbedx509.vcxproj]
C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\x509_crt.c(371): warning C4146: unary minus operator applied to unsigned type, result still unsigned [C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\mbedx509.vcxproj]
  x509_csr.c
  x509write_crt.c
  x509write_csr.c
  Generating Code...
Done Building Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\mbedx509.vcxproj" (default targets) -- FAILED.
Done Building Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\library\mbedtls.vcxproj" (default targets) -- FAILED.
Done Building Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\programs\aes\aescrypt2.vcxproj" (default targets) -- FAILED.
Done Building Project "C:\buildbot\mbedtls\any_-_polarssl_win32__msvc12_64_make_\build\ALL_BUILD.vcxproj" (default targets) -- FAILED.

Build FAILED.

Demoting the PR to needs work.

[simonbutcher]

@hanno-arm - I would suggest it's best to adopt @irwir's suggestion of:

ver_chain->items[i].flags = ~0;

[hanno-becker]

@sbutcher-arm For uniformity with the rest of the code, I will follow @rahmanih's suggestion of using (uint32_t) -1.

[hanno-becker]

@AndrzejKurek @mpg Could you please re-review?

[mpg]

LGTM

[irwir]

Good style would be to avoid type casts; particularly when a cast relies on internal representation of values.
It could be that ~0 != (unsigned)-1.

[hanno-becker]

@irwir That's true for unsigned, but for uint32_t we have ~0 == (uint32_t) -1.

[irwir]

For example, in MSVC/x86 uint32_t is unsigned long, and long is the same as int.
That means, uint32_t and unsigned are (almost) the same, while the missed point was internal representation of integer values.
How are negative signed values stored?.

[hanno-becker]

@irwir uint32_t is required to not have padding, in contrast to unsigned. That's why it makes a difference.

[irwir]

@hanno-arm

uint32_t is required to not have padding, in contrast to unsigned. That's why it makes a difference.

Sorry, I could not get this pharase about padding. Is this stated in C statdard?

[hanno-becker]

@irwir Yes, uintN_t doesn't have padding, see Section 7.18.1.1, or https://stackoverflow.com/questions/19779537/uint32-t-vs-uint-fast32-t-vs-uint-least32-t.

Some comments:

Good style would be to avoid type casts;

I disagree. What should be avoided are reinterpretation casts, i.e. such which take an internal presentation of a value of one type and view it as the representation of another type. Unless the standard mandates the internal presentation, this leads to nonportable code. Casts such as (unsigned) -1 are not of this type; they are static casts defined on values, and if a C implementation uses incompatible internal representations of integers and unsigned integers, it must perform the necessary transformation when implementing a typecast like the one above. For example, in one's complement, -1 = (111..10), while (unsigned) -1 = (111..11) (omitting potential padding bits).

In regards to your suggestion flag = ~0, it's actually not portable. The point is that ~ is not defined on values, but on internal presentations, requiring to flip every sign- and value-bit in the presentation of the value given. This has different effects on signed values depending on whether integers are represented through two's complement or one's complement. For example, in one's complement, you have ~0 == 0 in int (Note, here, that == performs a value comparison, not a presentation comparison). So, if unsigned flag; and flag = ~0, the computation of ~0 happens in int and on a one's complement machine leads to (a presentation of the value) 0, which is then value-converted to unsigned, which leads to flag = 0. What would be ok is flag = ~(unsigned) 0.

Finally, I was wrong saying that ~0 == (uint32_t) -1 but not necessarily ~0 == (unsigned) -1, alluding to the lack of padding bits in uint32_t. While it is true that uint32_t doesn't have padding bits while unsigned might have them, padding bits don't matter here as == is defined on values and not on internal presentations.

In summary: valid ( := portable ) options are flag = ~ (T) 0 and flag = (T) -1 for flag of any unsigned type T; but flag = ~0 isn't.

[irwir]

@hanno-arm, thanks for the answer.

Padding bits are a) optional; b) non-existent in most cases (7.18.1.3); and c) irrelevant because it does not change the value itself (as you mentioned). uint32_t also could be a mere typedef, and then native C types had no padding.

In regards to your suggestion flag = ~0, it's actually not portable. The point is that ~ is not defined on values, but on internal presentations, requiring to flip every sign- and value-bit in the presentation of the value given.

Quick look at the code in x509_crt.c suggests, that flags member is used as bit flags.
Then internal representation of integers does not matter.

Finally, I was wrong saying that ~0 == (uint32_t) -1 but not necessarily ~0 == (unsigned) -1, alluding to the lack of padding bits in uint32_t.

Will ~0 == (uint32_t) -1 be true for one's complement? Probably, no.

In summary: valid ( := portable ) options are flag = ~ (T) 0 and flag = (T) -1 for flag of any unsigned type T; but flag = ~0 isn't.

UINT32_MAX (7.18.2.1) might be the best if it existed in every platform.
Tricks with -1 depend on internal integer representation and thus non-portable.
~(uint32_t)0 should avoid most problems. Or maybe ~0ul, because 5.2.4.2.1 says that unsigned long should be able to store at least 32-bit values.

Edited out several incorrect statements.

[hanno-becker]

Hi @irwir,

Will ~0 == (uint32_t) -1 be true for one's complement? Probably, no.

No, it won't be true regardless of the unsigned integer type you take, because ~0 == 0 in this case.

Tricks with -1 depend on internal integer representation and thus non-portable.

No, it's not a trick that might or might not work depending on the C implementation - it's behavior entirely defined by the standard: The conversion (T) n of a signed integer of value n into an unsigned integer type T of range [0,N] has as its value the unique m such that 0 <= m < N and m - n is a multiple of N. In particular, the conversion of (T) -1 into the unsigned integer type T of range [0,N] has value N-1; i.e. (T) -1 == T_MAX.

[hanno-becker]

@irwir Your point would apply to code like int x=-1; unsigned y = *((unsigned*) &x);. This is indeed non-portable, the reason being the use of a reinterpretation cast, whose effect on values is implementation-dependent. But that's different from int x = -1; unsigned y = x; which involves a static cast whose effect on values is fully defined by the standard. One thing about C++ I quite like is that it makes these differences very visible through the explicit reinterpret_cast<T> and static_cast<T> annotations. In a previous project I grepped for reinterpret_cast and went through every use, and indeed some of them should have been static_casts and lead to compatibility bugs.

[mpg]

@irwir

Tricks with -1 depend on internal integer representation and thus non-portable.

I think the C standard defines very well what this operation (I disagree with calling it a trick) does.

1. Regarding the value, 6.3.1.3 (2) mandates that "[if the value can be represented by the new type] if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type" which it's clear that UINT32_MAX == (uint32_t) -1 holds on all compliant platforms.

2. Regarding the representation of value bits, 6.2.6.2 (1) mandates that "If there are N value bits, each bit shall represent a different power of 2 between 1 and 2**(N−1), so that objects of that type shall be capable of representing values from 0 to 2**N − 1 using a pure binary representation".

3. Regarding the eventuality of padding bits in the representation, as Hanno already mentionned, 7.18.1.1 (1) mandates that uint32_t has no padding bits.

4. So, both the value and the full representation of (uint32_t) -1 are perfectly well-defined by the C standard. This is one of the happy case where we don't have to worry about undefined behaviour, so let's enjoy it.

(Regarding casts, I fully agree with what Hanno wrote about the difference between unsafe reinterpret casts and safe static casts.)

[irwir]

@hanno-arm

No, it won't be true regardless of the unsigned integer type you take, because ~0 == 0 in this case.

~0 == 0 might be correct in implementations with support for a negative zero.

@mpg
I agree about representation.
However, there is only one kind of C casts, and that one is more powerfrill than any of C++'s (reinterpret_cast might require an addidtional const_cast).
Grep would be of little help in finding safe & static C casts.
On the subject of tricks.
The binary code would be the same, but constants such as 0xfffffffful or ~0ul better suit to bit masks pattern than a negative decimal value converted to unsigned.

[hanno-becker]

@AndrzejKurek Could you please re-review?

[hanno-becker]

Ping @sbutcher-arm for gatekeeping.