On 6/27/24 12:47 PM, Yann Ylavic wrote:
> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>> How about the below? I am yet undecided if I should follow the below
>>> approach to have
>>> two completely separate callbacks depending on the OpenSSL version or one
>>> with a lot of
>>> #If statements in it, but as much shared code as possible. Thoughts?
>>
>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>>
>> Something like the attached maybe? modssl_io_cb() will just consider
>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
>> than enough for debug logs..
>
> We could even log the real length still if it matters, see attached v2.
Thanks. Unfortunately the meaning of rc varies depending on if we have the ex
or the non ex callback.
This is not in the documentation but only in the OpenSSL code :-(.
Furthermore the processed amount of data is in *processed in the ex case
whereas in the non ex case it is in
rc. The intended amount of data to be processed is in len in the ex case and in
argi in the non ex case.
Hence I propose the patch below. Of course we can have a longer debate if the
len parameter to ssl_io_data_dump
should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to
the log might be bad.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918662)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,7 +2361,7 @@
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
- "| %04ld - <SPACES/NULS>", len + trunc);
+ "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len +
(size_t)trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-------------------------------------------------------------------------+");
}
@@ -2379,9 +2379,9 @@
conn_rec *c;
server_rec *s;
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
- (void)len;
- (void)processed;
+ (void)argi;
#endif
+ (void)argl;
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2391,7 +2391,20 @@
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+#define requested_len (len)
+#define actual_len (*processed)
+ /*
+ * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
+ * BIO_write_ex functions return value and not the one of
+ * BIO_read and BIO_write. Hence 0 indicates an error.
+ */
+ if (rc > 0) {
+#else
+#define requested_len ((size_t)argi)
+#define actual_len ((size_t)rc)
if (rc >= 0) {
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
if (argp != NULL)
@@ -2400,18 +2413,21 @@
dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+ "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
+ " bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
- (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ?
"to" : "from"),
+ actual_len, requested_len,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
if (*dump != '\0' && argp != NULL)
- ssl_io_data_dump(c, s, argp, rc);
+ ssl_io_data_dump(c, s, argp, actual_len);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem:
%pp]",
- MODSSL_LIBRARY_NAME, argi,
+ "%s: I/O error, %" APR_SIZE_T_FMT
+ " bytes expected to %s on BIO#%pp [mem: %pp]",
+ MODSSL_LIBRARY_NAME, requested_len,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
bio, argp);
}
Regards
Rüdiger
>
>>
>> Regards;
>> Yann.
