Skip to content

openssl: Check return value of SSL_CTX_set_alpn_protos() - #22996

Merged
ndossche merged 1 commit into
php:masterfrom
ndossche:ssl-esss-1
Aug 3, 2026
Merged

openssl: Check return value of SSL_CTX_set_alpn_protos()#22996
ndossche merged 1 commit into
php:masterfrom
ndossche:ssl-esss-1

Conversation

@ndossche

@ndossche ndossche commented Aug 2, 2026

Copy link
Copy Markdown
Member

Discovered by ESSS.

@bukka bukka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm looking at https://github.com/openssl/openssl/blob/77f492f29f882e07d57e16920138d991dc9af018/ssl/ssl_lib.c#L3870-L3882 it seems like it might be possible to make it fail (e.g. single letter value so test might be possible if I read it correctly.

@ndossche

ndossche commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Hmm looking at https://github.com/openssl/openssl/blob/77f492f29f882e07d57e16920138d991dc9af018/ssl/ssl_lib.c#L3870-L3882 it seems like it might be possible to make it fail (e.g. single letter value so test might be possible if I read it correctly.

Yeah, I added a test and adjusted the error type

@bukka bukka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

technically it could fail on memdup but such message wouldn't be probably visible anyway so it's better to keep like this.

@bukka

bukka commented Aug 3, 2026

Copy link
Copy Markdown
Member

Seems like a bug so should go to 8.4+

@ndossche

ndossche commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

technically it could fail on memdup but such message wouldn't be probably visible anyway so it's better to keep like this.

Yes, I agree, we can't distinguish what exactly failed at the call site so this is the pragmatic approach.

@ndossche

ndossche commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Seems like a bug so should go to 8.4+

This particular patch uses the new API php_stream_warn. So I'll merge this to master and then prepare a backport PR for 8.4.

@ndossche
ndossche merged commit 8fb6827 into php:master Aug 3, 2026
18 checks passed
ndossche added a commit that referenced this pull request Aug 4, 2026
* PHP-8.5:
  openssl: Check return value of SSL_CTX_set_alpn_protos() (#22996)
pull Bot pushed a commit to Alfaesal23/php-src that referenced this pull request Aug 4, 2026
pull Bot pushed a commit to Alfaesal23/php-src that referenced this pull request Aug 4, 2026
* PHP-8.4:
  openssl: Check return value of SSL_CTX_set_alpn_protos() (php#22996)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants