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.
Regards
Rüdiger
>
>
> 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.
>
