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

ssl-opt.sh no longer runs on OS X #2029

Closed
sbutcher-arm opened this issue Sep 26, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@sbutcher-arm
Copy link
Collaborator

commented Sep 26, 2018

Description

  • Type: Bug
  • Priority: Major

Bug

OS
OS X

mbed TLS build:
Version: mbedtls-2.13.0 and later

The test script ssl-opt.sh no longer runs on OS X, failing with the following output (this is an extract):

DTLS reordering: Buffer out-of-order handshake message on client ....... PASS
DTLS reordering: Buffer out-of-order handshake message fragment on client  PASS
./tests/ssl-opt.sh: line 176: [: //#define MBEDTLS_SSL_DTLS_MAX_BUFFERING             32768: integer expression expected
DTLS reordering: Buffer out-of-order hs msg before reassembling next ... PASS
./tests/ssl-opt.sh: line 176: [: //#define MBEDTLS_SSL_DTLS_MAX_BUFFERING             32768: integer expression expected
./tests/ssl-opt.sh: line 183: [: //#define MBEDTLS_SSL_DTLS_MAX_BUFFERING             32768: integer expression expected

The root cause, is the following introduction into ssl-opt.sh:

get_config_value_or_default() {
    NAME="$1"
    DEF_VAL=$( grep ".*#define.*${NAME}" ../include/mbedtls/config.h |
               sed 's/^.*\s\([0-9]*\)$/\1/' )
    ../scripts/config.pl get $NAME || echo "$DEF_VAL"
}

The use of sed with Linux/GNU platforms, yields the configured value for the configuration when it's commented out, whilst on OS X, it returns the entire line the configuration is on.

So, if the configuration is commented out, why do we have a test that uses it?

@hanno-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@sbutcher-arm There are two things to comment on:

  • The rationale to not ignore commented lines is that each numeric configuration option is listed as // #define NAME DEFAULT_VALUE in config.h with DEFAULT_VALUE being the default value defined in e.g. https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/ssl.h in case config.h doesn't overwrite it; so even if the line is commented out, we get the config value used by Mbed TLS by grepping for it. This property does of course not extend to an arbitrary config.h, which might remove the commented lines altogether, and a more robust means to determine configuration values is desirable - however, this would require inspecting also e.g. https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/ssl.h for the real default value of some options, take into account user-specified configuration files, re-defines of options, or similar matters. The present implementation of get_config_value_or_default() deliberately restricts to versions of config.h which, for each configuration option, either keep the // #define NAME DEFAULT_VALUE line, or replace it by a single #define NAME NEW_VALUE.

  • The ssl-opt.sh failure documented here is orthogonal and looks like it's due to a different version of sed on OS X.

@sbutcher-arm

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2018

Based on what you say @hanno-arm, when we fix the sed issue, I think we should add a comment in the same area describing the limitations of this approach. It's pragmatic, but fragile, and worth fixing in the long run.

The issue with sed has hit us before. I think this is the second or third bug we've had where BSD sed doesn't do the same thing as GNU sed.

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

ARM Internal Ref: IOTSSL-2554

@ciarmcom ciarmcom added the mirrored label Sep 26, 2018

@hanno-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@sbutcher-arm I fully agree.

@gilles-peskine-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

Given #2030, I think that this issue is moot because the corresponding shell functions should be removed altogether and replaced by a different approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.