On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> Thanks for the intense review. I hope I captured all stuff in the below.
Thanks for working on this.
> 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;
The problem is that some of the i,j,.. should apr_size_t too then.
I'd rather we use "int len" here because APR_INT32_MAX is really a
hard limit IMHO, we shouldn't take seconds/minutes to log things to
disk anyway, and it'd avoid thinking about overflows here and keep
things simple.
> @@ -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);
So "int dump_len" here should be (more than) enough?
> + 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;
Yann.
