Fix definition error for the deprecated zlib support if enabled

[jiblime]

When MBEDTLS_ZLIB_SUPPORT is defined and -DENABLE_ZLIB_SUPPORT=1 is enabled, mbedtls will build with zlib support but now has an issue starting with 2.19.1.

100% tests passed, 0 tests failed out of 85 for both the current development branch and 2.19.1

Previous implementations of zlib did not use session_negotiate: 2.17.0 2.18.1

https://github.com/ARMmbed/mbedtls/blob/mbedtls-2.19.1/library/ssl_tls.c#L1842

#if defined(MBEDTLS_ZLIB_SUPPORT)
                                  ssl->session_negotiate->compression,
#endif

Problem line here

I don't understand why not just combine it instead of making it implicit.

Fixes #2859 (edited by mpg for github keywords)

Despite it being deprecated, the test suite should allow zlib inclusion to be tested.

https://github.com/ARMmbed/mbedtls/blob/development/tests/Makefile#L60

Status

READY

Requires Backporting

NO - none of the LTS branches are affected (edited by mpg).

2.19.1 -- https://github.com/jiblime/mbedtls/blob/mbedtls-2.19.1-zlib-fix/library/ssl_tls.c#L1862

diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index a7facb81a..8893e2c3c 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -1859,7 +1859,7 @@ int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl )
 
     /* Allocate compression buffer */
 #if defined(MBEDTLS_ZLIB_SUPPORT)
-    if( session->compression == MBEDTLS_SSL_COMPRESS_DEFLATE &&
+    if( ssl->session_negotiate->compression == MBEDTLS_SSL_COMPRESS_DEFLATE &&
         ssl->compress_buf == NULL )
     {
         MBEDTLS_SSL_DEBUG_MSG( 3, ( "Allocating compression buffer" ) );

Todos

• Tests - added in a separate PR: Add zlib tests and fix runtime bug #2975
• Changelog updated

Steps to test or reproduce

Configure mbedtls with #define MBEDTLS_ZLIB_SUPPORT uncommented in include/mbedtls/config.h and -DENABLE_ZLIB_SUPPORT=1, then compile. Result:

net-libs/mbedtls-2.19.1/work/mbedtls-mbedtls-2.19.1/library/ssl_tls.c: In function ‘mbedtls_ssl_derive_keys’:
net-libs/mbedtls-2.19.1/work/mbedtls-mbedtls-2.19.1/library/ssl_tls.c:1862:9: error: ‘session’ undeclared (first use in this function)
 1862 |     if( session->compression == MBEDTLS_SSL_COMPRESS_DEFLATE &&
      |         ^~~~~~~

Reproducible always.

[mpg]

Thanks for your PR!

I used the following command to reproduce the issue and verify it's fixed by this PR: scripts/config.pl set MBEDTLS_ZLIB_SUPPORT && make lib.

• This fails with tag mbedtls-2.19.1 and the current development branch.
• This succeeds with this PR's branch as well as current mbedtls-2.16 and mbedtls-2.7 branches.

I'll be editing the PR description as we only backport bug fixes to our LTS branches (currently 2.16 and 2.7) and none of them is affected. I'll also update the reference to #2859 so that github can pick up that this PR fixes it.

[mpg]

The fix looks good to me.

However, please add an entry to the ChangeLog, in the Bugfix section for 2.20.0, along the lines of "Fix build failure with MBEDTLS_ZLIB_SUPPORT enabled. Reported by Jack Lloyd and fix submitted by <your name (and affiliation) or github handle or whatever your prefer>. Fixes #2859"

The PR is also missing non-regression tests, but I'll add them in a separate PR so that I can handle the backport (the bug fix itself doesn't need backporting as it doesn't affect the LTS branches, but we do want the test improvements backported to the LTS branches).

[gilles-peskine-arm]

In the interest of moving on with this PR, I've taken the liberty of pushing a changelog entry. Unfortunately, it's too late for 2.20.0, but we may issue a 2.20.1 soon.

@jiblime I used your GitHub handle by default in the ChangeLog file. Please let us know if you'd prefer to be acknowledge under a different name.

[mpg]

Thanks for adding the ChangeLog entry. LGTM.

[mpg]

The pr-merge job exited early due to a transient communication error with github, restarting it.

[mpg]

The pr-head job doesn't succeed because of issues independent from this PR that were fixed in development in the meantime. The pr-merge job can't run (and confirm my assertion in the previous sentence) due to merge conflicts in the ChangeLog. I'm rebasing to solve those issues.

[jiblime]

In the interest of moving on with this PR, I've taken the liberty of pushing a changelog entry. Unfortunately, it's too late for 2.20.0, but we may issue a 2.20.1 soon.

@jiblime I used your GitHub handle by default in the ChangeLog file. Please let us know if you'd prefer to be acknowledge under a different name.

I'm very sorry for the late reply. It's been a while since I've looked at what I've done and I am unclear on what is going wrong now. Regarding upstreaming this, is there an issue now that the current version does not work with it?

[mpg]

Hi @jiblime ! As far as I can see, nothing's going wrong with the code and everything is in order on this side. (Previous issues were with tests we added in the CI in the meantime for problems we solved in the meantime, all independent from your changes.)

The only question left is whether you're happy with the ChangeLog entry we wrote or if you'd prefer to be referred to by something else that you github handle (for example, legal name). It's up to you and unless you tell us otherwise we'll keep the current entry.

[mpg]

The CI fully passed the pr-head job, but the pr-merge job had one failure in one of the ssl-opt.sh tests, which looked spurious. Restarting the job.

[mpg]

The pr-merge job can no longer run, due to a conflict in the ChangeLog, but since the pr-head job passed, I'm removing the label "needs: CI" and marking this as ready for merge.