On 7/1/24 3:01 PM, Ruediger Pluem wrote:
>
>
> On 6/27/24 3:48 PM, Ruediger Pluem wrote:
>>
>>
>> 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.
>
> Any further comments on whether we should limit the dump to APR_INT32_MAX or
> not? I would guess that it will not matter
> in practice, but I am still open to it.
> BTW: I guess
>
> int len = data_len & APR_INT32_MAX;
>
> would be wrong. It would need to be
>
> int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
>
> instead.
Updated patch.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918813)
+++ 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,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)
- 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);
}
}
return rc;
+#undef requested_len
+#undef actual_len
}
static APR_INLINE void set_bio_callback(BIO *bio, void *arg)
Regards
Rüdiger
