Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport 2.28: Support larger integer test arguments #7503

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Apr 26, 2023

Backport of #6716. Mostly straightforward, but there's an extra commit and some minor differences from the original:

  • "Remove mbedtls_ prefix from bignum test cases": extra commit, backported (redone) from Bignum: Montgomery multiplication from bignum prototype #6083, to facilitate this and future backports.
  • "Remove declarations of the nonstandard function strcasecmp"
    • tests/suites/helpers.function: different context around the removal of #include <strings.h>.
  • "Remove stdint.h substitute for older MSVC":
    • tests/suites/helpers.function: trivial conflict due to adjacent changes (removed block next to code that only exists in 2.28).
  • "Simplify string escapes":
    • tests/suites/test_suite_x509parse.data: trivial conflict due to adjacent changes (different dependencies adjacent to the changed data lines).
  • "Support different types in the parameter store":
    • tests/suites/helpers.function: trivial conflict due to a gratuitous difference in the order of includes.
    • tests/include/test/arguments.h: MBEDTLS_CONFIG_FILE or mbedtls/config.h in 2.28 corresponds to mbedtls/build_info.h in development.
    • visualc/VS2010/mbedTLS.vcxproj: update generated file.
  • "parse_function_arguments: stricter type parsing":
    • tests/suites/test_suite_psa_crypto.function: copy_success does not have lifetime arguments in 2.28.

Gatekeeper checklist

Align 2.28 with development to make backports easier.

Signed-off-by: Gilles Peskine <[email protected]>
We're using the non-standard function strcasecmp() just so that the case
of digits beyond 9 can be different in the library and in the test data.
Use matching case in the test data, and use a standard function for the
comparison.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added enhancement needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) component-test Test framework and CI scripts priority-medium Medium priority - this can be reviewed as time permits needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Apr 26, 2023
lpy4105
lpy4105 previously approved these changes Apr 28, 2023
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared with patches that introduced in development branch, looks good to me.

It is no longer used.

Signed-off-by: Gilles Peskine <[email protected]>
We now require at least Visual Studio 2013, which has stdint.h per
 https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2013/y4hta57s(v=vs.120)
so the workaround to define C99 types on pre-C99 MSVC is no longer needed.

Signed-off-by: Gilles Peskine <[email protected]>
This is just a quick improvement, not meant to tackle the problem as a
whole.

Signed-off-by: Gilles Peskine <[email protected]>
No intended behavior change. This commit is mainly to satisfy pylint, which
complains that gen_from_test_data now has too many variables. But it's a
good thing anyway to make the function a little more readable.

Signed-off-by: Gilles Peskine <[email protected]>
Treat backslash as a universal escape character: "\n" is a newline,
backslash escapes any non-alphanumeric character.

This affects some test cases that had "\," standing for backslash-comma.
With the new uniform treatment of backslashes, this needs to be "\\,".

Signed-off-by: Gilles Peskine <[email protected]>
In the .datax parser, since we're calling strtol() anyway, rely on it for
verification. This makes the .datax parser very slightly more
liberal (leading spaces and '+' are now accepted), and changes the
interpretation of numbers with leading zeros to octal.

Before, an argument like :0123: was parsed as decimal, but an argument like
:0123+1: was parsed as a C expression and hence the leading zero marked an
octal representation. Now, a leading zero is always interpreted according to
C syntax, namely indicating octal. There are no nonzero integer constants
with a leading zero in a .data file, so this does not affect existing test
cases.

In the .datax generator, allow negative arguments to be 'int' (before, they
were systematically treated as 'exp' even though they didn't need to be).

In the .datax parser, validate the range of integer constants. They have to
fit in int32_t. In the .datax generator, use 'exp' instead of 'int' for
integer constants that are out of range.

Signed-off-by: Gilles Peskine <[email protected]>
The test framework stores size_t and int32_t values in the parameter store
by converting them all to int. This is ok in practice, since we assume int
covers int32_t and we don't have test data larger than 2GB. But it's
confusing and error-prone. So make the parameter store a union, which allows
size_t values not to be potentially truncated and makes the code a little
clearer.

Signed-off-by: Gilles Peskine <[email protected]>
Internal refactoring only, no behavior change.

Signed-off-by: Gilles Peskine <[email protected]>
Internal refactoring only, no behavior change.

Signed-off-by: Gilles Peskine <[email protected]>
Use normalization the equality comparisons instead of loose regular
expressions to determine the type of an argument of a test function.

Now declarations are parsed in a stricter way: there can't be ignored junk
at the beginning or at the end. For example, `long long unsigned int x`
was accepted as a test function argument (but not `long long unsigned x`),
although this was misleading since the value was truncated to the range of
int. Now only recognized types are accepted.

The new code is slightly looser in that it accepts `char const*` as well as
`const char*`.

Signed-off-by: Gilles Peskine <[email protected]>
Change the type of signed integer arguments from int32_t to intmax_t.
This allows the C code to work with test function arguments with a range
larger than int32_t. A subsequent commit will change the .datax generator
to support larger types.

Signed-off-by: Gilles Peskine <[email protected]>
Now that the C code supports the full range of intmax_t, allow any size of
signed integer type in the .data file parser.

Signed-off-by: Gilles Peskine <[email protected]>
Now that the test framework can pass arbitrary values of type
mbedtls_mpi_sint, just do that.

Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
The test framework used to treat them specially (but no longer does). Add
these test cases as non-regression for how the test framework allows "?"
and especially "??" (which I think in the very distant path needed special
handling because the test data was embedded in a .c file, and thus ?? could
be interpreted as the prefix of a trigraph).

Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Copy link

@yanrayw yanrayw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yanrayw yanrayw added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 4, 2023
@gilles-peskine-arm gilles-peskine-arm merged commit 5ead738 into Mbed-TLS:mbedtls-2.28 May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-test Test framework and CI scripts enhancement needs-ci Needs to pass CI tests priority-medium Medium priority - this can be reviewed as time permits size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants