Skip to content

Conversation

zeff-ir
Copy link

@zeff-ir zeff-ir commented Sep 22, 2025

What is fixed

SSL_write() returns int and uses negative values to indicate errors.
The variable sent in ext/ftp/ftp.c was declared as size_t, which
caused negative return values to be converted into large positive numbers.

Why it matters

Although this value is later passed to SSL_get_error(), the OpenSSL API
requires the original return value of SSL_write() to be used. Passing a
converted unsigned value may lead to incorrect error handling and potentially
report a failed write as a success.

Why change is safe

While the implicit int → size_t → int conversion is “usually” safe on most
compilers, it relies on implementation-defined behavior. Using a signed int
makes the code clearer, avoids subtle portability issues, and directly matches
the return type contract of SSL_write().

How it is fixed

Changed the type of sent from size_t to int.

@TimWolla
Copy link
Member

Could probably be SSL_write_ex instead, which has a size_t out parameter.

@zeff-ir
Copy link
Author

zeff-ir commented Sep 22, 2025

I chose this approach for easier backporting to PHP 8.3 (which supports OpenSSL ≥ 1.0.2 and is my primary focus). Since SSL_write_ex is only available from 1.1.1.

Could probably be SSL_write_ex instead, which has a size_t out parameter.

I can rework the PR if you prefer that variant for master.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

This is also wrong as my_send_wrapper_with_restart returns an ssize_t.
Edit: nvm reading on mobile is hard. The max size that can be sent shouldn't actually be a size_t in the first place, as that can truncate...

SSL_write() returns int (may be negative). Store its result in int
instead of size_t. Also cap the send length to INT_MAX before
SSL_write() to avoid truncation; do the same in the non-SSL path
and cast the return to int. Include <limits.h> for INT_MAX.

Signed-off-by: Denis Sergeev <zeff@altlinux.org>
@zeff-ir zeff-ir force-pushed the fix-signed-to-unsigned-ftp branch from db286fe to 269f975 Compare September 24, 2025 10:41
@zeff-ir
Copy link
Author

zeff-ir commented Sep 24, 2025

This is also wrong as my_send_wrapper_with_restart returns an ssize_t.
Edit: nvm reading on mobile is hard. The max size that can be sent shouldn't actually be a size_t in the first place, as that can truncate...

Thanks for the review. I’ve pushed a new version.

@zeff-ir zeff-ir requested a review from nielsdos September 26, 2025 06:59
@bukka
Copy link
Member

bukka commented Oct 1, 2025

OpenSSL 1.1.1 is minimum since 8.4 so you can use SSL_write_ex if you target 8.4+ which should be fine for this. Not sure if we should even treat it as a bug but I'm fine either way.

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.

5 participants