Skip to content

configure: fix broken m4 syntax in TLS options#9344

Closed
cgpe-a wants to merge 1 commit intocurl:masterfrom
cgpe-a:master
Closed

configure: fix broken m4 syntax in TLS options#9344
cgpe-a wants to merge 1 commit intocurl:masterfrom
cgpe-a:master

Conversation

@cgpe-a
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants