-
Notifications
You must be signed in to change notification settings - Fork 8k
ftp: use int for sent to preserve SSL_write() #19912
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
base: master
Are you sure you want to change the base?
Conversation
Could probably be |
I chose this approach for easier backporting to PHP 8.3 (which supports OpenSSL ≥ 1.0.2 and is my primary focus). Since
I can rework the PR if you prefer that variant for master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this 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>
db286fe
to
269f975
Compare
Thanks for the review. I’ve pushed a new version. |
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. |
What is fixed
SSL_write()
returnsint
and uses negative values to indicate errors.The variable
sent
inext/ftp/ftp.c
was declared assize_t
, whichcaused 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 APIrequires the original return value of
SSL_write()
to be used. Passing aconverted 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
fromsize_t
toint
.