Use 16-bit unsigned integers in ssl_srv.c

[irwir]

Description

This continues what was done in already merged PR #2856 and PR #3150 - use shorter integers if higher bits were never used.

Additional tiny improvements/optimizations: two cases of moving around common expressions (at lines 1440 and 1509 in the new code) and corrected type cast in line 2228.

Status

READY

Requires Backporting

NO
Which branch?
development

Migrations

NO

[danh-arm]

Hi @irwir. Thanks for this but the PR builds with warnings which are treated as errors in some configs:

ssl_srv.c: In function ‘ssl_parse_servername_ext’:
ssl_srv.c:103:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if( servername_list_size + 2 != len )
                                  ^
ssl_srv.c: In function ‘ssl_parse_signature_algorithms_ext’:
ssl_srv.c:265:31: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if( sig_alg_list_size + 2 != len ||
                               ^
ssl_srv.c: In function ‘ssl_parse_supported_elliptic_curves’:
ssl_srv.c:339:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if( list_size + 2 != len ||

Also, this will need a changelog entry.

[irwir]

Thanks, @danh-arm.

the PR builds with warnings which are treated as errors in some configs:

Now the issues should be fixed.
Parenthses in the line 1704 were inconsistent with the rest of the file.

Also, this will need a changelog entry.

Done, please see if it needs improvements.

[danh-arm]

CI passed (failures not relevant) but don't think this is ready yet.

[danh-arm]

Changing the type of servername_list_size doesn't really make sense if you have to then do a cast. You can't change the type of len since it's in the function spec so it's not worth changing this instance.

[irwir]

The key to this change is the previous line - two bytes make a 16-bit unsigned integer only; no additional space is required while size_t could be 32 or 64 bits.
As for type casts - you pointed out this issue previously, and its was necessary to suppress the compiler warning.
This cast should not generate additional code, because internally compiler would do about the same - expand 16 bits to integer size, and to size_t to compare.

[danh-arm]

It wasn't me who pointed that out previously but anyway, in that earlier PR the cast was from a larger type to a smaller one whereas here you're casting to a larger type, which presumably will undo any code size reduction by declaring the smaller type? Have you actually demonstrated this improves code size in some cases?

[irwir]

here you're casting to a larger type

The existing code has a cast to a larger type too, an implicit one:
servername_list_size = (size_t)( ( buf[0] << 8 ) | ( buf[1] ) );

[danh-arm]

Right. I'm not so much concerned about whether the cast is implicit or explicit. I'm more concerned the change won't improve the code, whether it's readability or code size or something else.

[gilles-peskine-arm]

(I haven't followed all the discussions in this PR) Code size at this micro level is very dependent on the target architecture as well as the compiler version and the surrounding code. It can happen that using a 16-bit variable to store a 16-bit value is a win if the variable has its own register, but a loss if the variable is spilled to memory, which is highly sensitive to how many registers are available at this point in the code and how the compiler uses them.

x86_64 code size is not that great an indicator because it's a 64-bit platform and code size is usually not an issue there. When I'm concerned about code size, I do a Cortex-M0+ build: that's the second-smallest Arm platform, and the smallest (Cortex-M0).

make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar LD=arm-none-eabi-ld
size library/*.o

About this specific line, a way to avoid a cast is

    if( servername_list_size + 2u != len )

I'd prefer to avoid a cast because a cast tells the reader “watch out, something fancy is happening here”, but nothing remarkable is happening on this line: there's no risk of overflow or truncation. The reason for the warning in 919af9b is that an unsigned 16-bit type got widened to int (because it fit) which then got wrapped around to a size_t, and the compiler analyzed the wrapped-around part independently from the widening part and is telling us that negative values could suffer, but thanks to the first part we know that negative values are impossible. All of which the reader shouldn't have to care about, because the only reason negative numbers started popping up was the use of a type (uint16_t) that's smaller than int and C's promotion rules kicking in.

[irwir]

@gilles-peskine-arm
Thanks for the idea; I made these changes and eliminated type casts.
On the subject of code size, it might be interesting if you could find time and check what it gets for these Cortex processor. Just for this single type change at line 90 to uint16_t.
For x86 and x64 code optimizer in MSVC is not doing a great job, and code with size_t was shorter.

[gilles-peskine-arm]

Compiler:

arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q3-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]

Flags: -Os -mthumb -mcpu=cortex-m0plus
Library configuration: scripts/config.py baremetal

Code size of ssl_srv.o (text column of size library/ssl_srv.o):

• Originally (5cac337): 20102
• Current version of this PR (a7025a2): 20102
• Current version, but with size_t instead of uint16_t at line 90: 20106

I haven't analyzed the generated assembly.

[irwir]

Thanks for the test.
Your numbers together with my data confirm that the difference is small and compiler-specific. Hence, the code size should be of a minor importance to prefer size_t to uint16_t or vice versa.

[irwir]

By the way, there was no All checks have passed messages for a long time. Good to see it fixed.

[danh-arm]

Ditto - this instance not worth fixing

[irwir]

The reasons are the same as above.

[danh-arm]

Ditto - this instance not worth fixing

[irwir]

Once again, the same as above.

[danh-arm]

Ditto - this instance not worth fixing

[irwir]

Just a little below, at line 3657 the len variable is used as 16-bit unsigned integer and therefore we would never need the whole size_t.
Yet again, type cast is only necessary to suppress a compiler warning.

[danh-arm]

Here, I accept the cast to a smaller type is OK, assuming the length is always represented in less than 16-bits. Even so, it could do with a comment like the earlier PR referring to a spec that validates this assumption.

[irwir]

a comment like the earlier PR

This is interesting. A value could be silently truncated to two bytes, but explicit type cast always raised questions.
According to comments, the returned value is the size of public_key in bytes. 16 bits would allow 2^19-1 bits for a key.
Would you mind suggesting a good comment line?

[danh-arm]

Hmm, I'm not the best person to suggest something here. @mpg - can you suggest a comment to use? Presumably we can assume something about the TLS buffer structure format?

[gilles-peskine-arm]

At this point, it isn't about TLS, it's about what mbedtls_pk_get_len can return. The maximum is MBEDTLS_MPI_MAX_SIZE, reached for an RSA key, and that value is 1024 by default and we say “Maximum tested: 2048 bytes” in include/mbedtls/config.h. That's RSA-16384 which is already slower than most people's patience.

Until post-quantum crypto changes the deal, I'm perfectly happy with assuming that key sizes in bits (let alone in bytes) fit in 16 bits. There's other code in the library that makes this assumption. I'm ok with not checking this assumption in TLS code.

[irwir]

Thanks for reviewing.
The general idea is to treat most of the cases where 2 bytes are read or written in the same style.
Additional type casts were necessary for suppressing compiler warnings.
That was partially implemented in mentioned PRs that were already merged, and used type casts too for the same reasons.
Depending on compiler and processor architecture, this might produce slightly shorter code and decrease the amount of used memory.

[danh-arm]

Won't the cast to a larger type here undo the saving by declaring a smaller type?

[gilles-peskine-arm]

What is the goal of this change? If it's about code size optimization, the results are inconclusive, but probably below the threshold of being worth the effort.

[hanno-becker]

I agree with @gilles-peskine-arm. While there can be a benefit in fine tuning the choice of integer type, I believe that if this is to be attempted, one should consider a general approach that applies throughout the library, and which also allows simple change of types for experimentation.

I played with this in the prototype MPS implementation, e.g. here by introducing typedefs for various kinds of integers and instantiating it with (a) natural machine width, (b) minimal width, e.g. uint8_t, (c) optimal type with required width, e.g. uint8fast_t, and I think the differences were quite minor, but I'd need to check again to be sure.

[irwir]

What is the goal of this change? If it's about code size optimization

Uniform approach was the goal, not code size.
I tried to reinforce this in the additional commit.

[irwir]

An additional commit that modifies ssl_cli.c in the same way.

[gilles-peskine-arm]

As a general matter, using 16-bit types for 16-bit values is potentially risky because as soon as you add them, you need to check for overflow. In much of the code around the TLS protocols, we're lucky that sizes are limited by the protocol to 24 bits or 16 bits, so if we use 32-bit types to manipulate variables, we don't risk overflows. A buffer size should be stored in a 16-bit type only if there's a good reason for it.

Furthermore using types that are less wide than int is generally risky in C due to promotions. It's easy to miss that x + 0 might not actually be equivalent to x because a promotion snuck in. So generally smaller types should be avoided for anything that will be involved in arithmetic. There are a lot of places where uint8_t is the right type because it's the type of a byte. Uses for uint16_t are rarer. It can be the right type to indicate that a value is known to fit in 16 bits, for example when preparing to write it out as two bytes. But I don't think it's a good idea in parsing code.

Using types that are smaller than the word size is typically a small performance and code size hit in terms of register manipulations: it tends to require additional fetches, extensions and bitwise and. They can save memory bandwidth, but this only matters if there's a non-negligible amount of variables involved. So generally speaking uint16_t arrays or uint16_t in data structures which are likely to be instantiated many times are performance wins, but uint16_t local variables are bad for performance and code size.

For these reasons, I am very wary of this pull request, and would need a much stronger reason than “uniform approach” to justify it. Furthermore, the work and the review have already taken a fairly long time that I think would be better spent elsewhere.

I've recorded my opinion as “request changes” and not closed the PR because I'm open to having my mind changed. But as it stands I'm opposed to the objective of the PR, not just to specific issues in the code.

[irwir]

@gilles-peskine-arm
Thanks for the review.

As a general matter, using 16-bit types for 16-bit values is potentially risky because as soon as you add them, you need to check for overflow.

Generally any integer type with fixed number of bits can overflow or wrap around.

It was already pointed out, the existing code assumes that it was safe to manipulate size_t numbers and then put the result into 16 bits.
Does it mean there was presumably no potential risks of truncation even for malformed data in 64-bit architecture?
Absense of checks implies that the result always had to be correct.
But in that case, no issues should be possible with short integers.

Otherwise, the issue was always here, and shorter data type would replace a truncation error with an unsigned wraparound.
The 16-bit resulting value would be the same, by the way.

A buffer size should be stored in a 16-bit type only if there's a good reason for it.

The existing library code implements the pertinent RFCs so that the code reads and writes 16-bit lengths.
That is, 16 bits is the natural size here.
It creates no risks in reading 16 bits of data into 16-bit variable or in writing such variable into 16 bits.

There are a lot of places where uint8_t is the right type because it's the type of a byte.

These types could be the right ones only when there are no more than 8 meaningful bits of data.
In this PR uint16_t is used for 16-bit values, so it could be considered as the right.

[danh-arm]

Sorry but I'm going to close this PR. It's taken too much review bandwidth for little or no gain. Before accepting changes like this, we would need a clear cost/benefit analysis, considering changes across the whole codebase. If needed, the mailing list is the best place to continue this discussion.

As a general matter, using 16-bit types for 16-bit values is potentially risky because as soon as you add them, you need to check for overflow.

Generally any integer type with fixed number of bits can overflow or wrap around.

True but I think the point Gilles was making is that if code generally fails to check for overflow, moving to a shorter type is more likely to trigger problems, though not in this specific case.