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

configure: fix broken m4 syntax in TLS options #9344

Closed
wants to merge 1 commit into from

Conversation

cgpe-a
Copy link
Contributor

@cgpe-a cgpe-a commented Aug 22, 2022

Commit b589696 added lines to some shell within AC_ARG_WITH macros, but
inadvertently failed to move the final closing ). e.g.:

OPT_RUSTLS=no
AC_ARG_WITH(rustls,dnl
AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls")
    experimental="$experimental rustls"
  fi

This generates a broken looking configure script:

# Check whether --with-rustls was given.
if test "${with_rustls+set}" = set; then :
  withval=$with_rustls; OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE
else
  }rustls"
fi

    experimental="$experimental rustls"
  fi

But amazingly, it executes OK, due to balancing " quotes and if/fi, as
you can see if you reformat the white space:

# Check whether --with-rustls was given.
if test "${with_rustls+set}" = set; then :
  withval=$with_rustls; OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE else }rustls"
  fi

  experimental="$experimental rustls"
fi

We move the closing ) back to the end of the section of script:

OPT_RUSTLS=no
AC_ARG_WITH(rustls,dnl
AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
    experimental="$experimental rustls"
  fi
)

However, with this change alone the generated configure script is still broken:

# Check whether --with-rustls was given.
if test "${with_rustls+set}" = set; then :
  withval=$with_rustls; OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE
else
  }rustls"
    experimental="$experimental rustls"
  fi

fi

This is because there is an unquoted comma in the script,
unintentionally separating AC_ARG_WITH arguments. We need to quote the
script section using braces:

OPT_RUSTLS=no
AC_ARG_WITH(rustls,dnl
AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),[
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
    experimental="$experimental rustls"
  fi
])

The generated script now has sane shell syntax:

# Check whether --with-rustls was given.
if test "${with_rustls+set}" = set; then :
  withval=$with_rustls;
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
    experimental="$experimental rustls"
  fi

fi

However, the 'test -z "TLSCHOICE" ||' part is a no-op, the same as
'false ||'. Perhaps it was cut-and-pasted from somewhere else, and a
dollar before TLSCHOICE was accidentaly deleted. However, with that
change, TLSCHOICE would never be set.

The code on the rhs of the || makes sense - it appends the new value,
separating it from any previous value with ", ". So, we can simplify by
removing the no-op test. This gives us:

OPT_RUSTLS=no
AC_ARG_WITH(rustls,dnl
AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),[
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
    experimental="$experimental rustls"
  fi
])

Generating:

# Check whether --with-rustls was given.
if test "${with_rustls+set}" = set; then :
  withval=$with_rustls;
  OPT_RUSTLS=$withval
  if test X"$withval" != Xno; then
    TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
    experimental="$experimental rustls"
  fi

fi

As far as I can tell, the quoting problem and no-op test have been there
for as long as this code has existed.

So, if these problems have been around for a while, how did I find them?
Only because I did a configure including these options:

$ ./configure --with-openssl --without-rustls
  SSL:              enabled (OpenSSL)

and got this diagnostic at the end:
WARNING: rustls enabled but marked EXPERIMENTAL. Use with caution!

I've tested these changes by running various combinations of options,
and checking that the diagnostic at the end prints correctly. e.g.:

$ ./configure --without-ssl
  SSL:              no      (--with-{openssl,gnutls,nss,mbedtls,wolfssl,schannel,secure-transport,amissl,bearssl,rustls} )

$ ./configure --with-openssl
  SSL:              enabled (OpenSSL)

$ ./configure --with-openssl --with-gnutls
  SSL:              enabled (OpenSSL, GnuTLS)

Commit b589696 added lines to some shell within AC_ARG_WITH macros, but
inadvertently failed to move the final closing ). e.g.:

    OPT_RUSTLS=no
    AC_ARG_WITH(rustls,dnl
    AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls")
        experimental="$experimental rustls"
      fi

This generates a broken looking configure script:

    # Check whether --with-rustls was given.
    if test "${with_rustls+set}" = set; then :
      withval=$with_rustls; OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE
    else
      }rustls"
    fi

        experimental="$experimental rustls"
      fi

But amazingly, it executes OK, due to balancing " quotes and if/fi, as
you can see if you reformat the white space:

    # Check whether --with-rustls was given.
    if test "${with_rustls+set}" = set; then :
      withval=$with_rustls; OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE else }rustls"
      fi

      experimental="$experimental rustls"
    fi

We move the closing ) back to the end of the section of script:

    OPT_RUSTLS=no
    AC_ARG_WITH(rustls,dnl
    AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
        experimental="$experimental rustls"
      fi
    )

However, with this change alone the generated configure script is still broken:

    # Check whether --with-rustls was given.
    if test "${with_rustls+set}" = set; then :
      withval=$with_rustls; OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE
    else
      }rustls"
        experimental="$experimental rustls"
      fi

    fi

This is because there is an unquoted comma in the script,
unintentionally separating AC_ARG_WITH arguments. We need to quote the
script section using braces:

    OPT_RUSTLS=no
    AC_ARG_WITH(rustls,dnl
    AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),[
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
        experimental="$experimental rustls"
      fi
    ])

The generated script now has sane shell syntax:

    # Check whether --with-rustls was given.
    if test "${with_rustls+set}" = set; then :
      withval=$with_rustls;
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        test -z "TLSCHOICE" || TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
        experimental="$experimental rustls"
      fi

    fi

However, the 'test -z "TLSCHOICE" ||' part is a no-op, the same as
'false ||'. Perhaps it was cut-and-pasted from somewhere else, and a
dollar before TLSCHOICE was accidentaly deleted. However, with that
change, TLSCHOICE would never be set.

The code on the rhs of the || makes sense - it appends the new value,
separating it from any previous value with ", ". So, we can simplify by
removing the no-op test. This gives us:

    OPT_RUSTLS=no
    AC_ARG_WITH(rustls,dnl
    AS_HELP_STRING([--with-rustls=PATH],[where to look for rustls, PATH points to the installation root]),[
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
        experimental="$experimental rustls"
      fi
    ])

Generating:

    # Check whether --with-rustls was given.
    if test "${with_rustls+set}" = set; then :
      withval=$with_rustls;
      OPT_RUSTLS=$withval
      if test X"$withval" != Xno; then
        TLSCHOICE="${TLSCHOICE:+$TLSCHOICE, }rustls"
        experimental="$experimental rustls"
      fi

    fi

As far as I can tell, the quoting problem and no-op test have been there
for as long as this code has existed.

So, if these problems have been around for a while, how did I find them?
Only because I did a configure including these options:

    $ ./configure --with-openssl --without-rustls
      SSL:              enabled (OpenSSL)

and got this diagnostic at the end:
      WARNING:  rustls enabled but marked EXPERIMENTAL. Use with caution!

I've tested these changes by running various combinations of options,
and checking that the diagnostic at the end prints correctly. e.g.:

    $ ./configure --without-ssl
      SSL:              no      (--with-{openssl,gnutls,nss,mbedtls,wolfssl,schannel,secure-transport,amissl,bearssl,rustls} )

    $ ./configure --with-openssl
      SSL:              enabled (OpenSSL)

    $ ./configure --with-openssl --with-gnutls
      SSL:              enabled (OpenSSL, GnuTLS)
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

👍 nice work!

@bagder bagder added the build label Aug 22, 2022
@bagder
Copy link
Member

bagder commented Aug 22, 2022

Thanks!

@bagder bagder closed this in a8f52ce Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants