On 7/3/24 11:01 AM, Yann Ylavic wrote:
> On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>> On 7/3/24 2:59 AM, Yann Ylavic wrote:
>>>> On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>>>>
>>>>> Updated patch.
>> []
>>>>> const char *dump = "";
>>>>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>>>>> if (argp != NULL)
>>>>> @@ -2400,23 +2413,28 @@
>>>>> 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)
>
> Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
> save the call when ssl_io_data_dump() is a noop anyway.
>
>>>>> - ssl_io_data_dump(c, s, argp, rc);
>>>>> + ssl_io_data_dump(c, s, argp, actual_len);
>>>> And here:
>>>> long dump_len = (actual_len >= APR_UINT16_MAX
>>>> ? APR_UINT16_MAX
>>>> : actual_len);
>>>
>>> Hm. As you point out: SSL records should be of limited length anyway, but
>>> if we really truncate the output shouldn't we log this?
>>> A log message like: Dump truncated to first XXX bytes?
>>
>> Something like this to initialize "dump" above?
>>
>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>> if (argp == NULL)
>> dump = "(Oops, no memory buffer?)";
>> else if (actual_len > MODSSL_IO_DUMP_MAX)
>> dump = "(BIO dump follows, truncated to "
>> APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
>> else
>> dump = "(BIO dump follows)";
>> }
>>
>> where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
>> everywhere eventually.
>>
>>>
>>>> ssl_io_data_dump(c, s, argp, dump_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);
>>>>> }
>>>>> }
>>>>> return rc;
>>>>> +#undef requested_len
>>>>> +#undef actual_len
>>>>> }
>>>> ?
>
Thanks for the intense review. I hope I captured all stuff in the below.
I intentionally left the 'len' parameter of ssl_io_data_dump an apr_size_t in
case we ever get to the conclusion that we want to
make MODSSL_IO_DUMP_MAX larger. It will fit then automatically.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918869)
+++ 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, apr_size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,11 +2361,13 @@
}
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,
"+-------------------------------------------------------------------------+");
}
+#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX
+
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
size_t len, int argi, long argl, int rc,
@@ -2379,9 +2381,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,27 +2393,58 @@
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
- if (rc >= 0) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ apr_size_t requested_len = len;
+ /*
+ * 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.
+ */
+ int ok = (rc > 0);
+#else
+ apr_size_t requested_len = (apr_size_t)argi;
+ int ok = (rc >= 0);
+#endif
+ if (ok) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ apr_size_t actual_len = *processed;
+#else
+ apr_size_t actual_len = (apr_size_t)rc;
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
- if (argp != NULL)
+ if (argp == NULL)
+ dump = "(Oops, no memory buffer?)";
+ else if (actual_len > MODSSL_IO_DUMP_MAX)
+ dump = "(BIO dump follows, truncated to "
+ APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
+ else
dump = "(BIO dump follows)";
- else
- 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);
+ /*
+ * *dump will only be != '\0' if
+ * APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)
+ */
+ if (*dump != '\0' && argp != NULL) {
+ apr_size_t dump_len = (actual_len >= MODSSL_IO_DUMP_MAX
+ ? MODSSL_IO_DUMP_MAX
+ : actual_len);
+ ssl_io_data_dump(c, s, argp, dump_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
