Fix allocation size overflow guards in ap_proxy_canonenc_ex and ap_make_full_path

[SABITHSAHEB]

Two ap_assert() guards on allocation sizes:

• ap_proxy_canonenc_ex(): len is an int, so 3 * len + 1 is evaluated in int arithmetic and overflows for len > INT_MAX / 3. The check now stays in the int domain: len >= 0 && len <= (INT_MAX - 1) / 3.
• ap_make_full_path(): len2 is bounded before it is subtracted, so the limit on len1 cannot itself wrap.

Checked with -std=c89 -Werror=declaration-after-statement.

[notroj]

Using ap_assert in these cases is correct, don't remove that, if these paths are reachable you are just introducing some new error path which would crash differently anyway. Don't add new such paths anywhere, just use ap_assert throughout.

[SABITHSAHEB]

Makes sense, reverted to ap_assert across the board: restored the ones I'd removed and used it for the new checks too instead of the error-return paths. I also pulled the unrelated mod_core atoi changes and the cookie reverse-map sizing churn so this stays focused on the escape helpers. Pushed.

[SABITHSAHEB]

any update?

[notroj]

mod_rewrite.c: In function 'escape_backref':
mod_rewrite.c:727:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  727 |     char *copy = apr_palloc(p, 3 * plen + 1);
      |     ^~~~

this doesn't compile in C90 in various places. Build with ./configure --enable-maintainer-mode

[SABITHSAHEB]

Fixed and pushed. The offenders were escape_backref in mod_rewrite.c and ftp_escape_globbingchars in mod_proxy_ftp.c, where the new declarations ended up after statements; moved them up into the declaration blocks. Rebuilt with --enable-maintainer-mode and the touched files compile clean now under -Wdeclaration-after-statement. The only failure left in my tree was the pre-existing SSL_get_handshake_rtt apr_uint64_t vs uint64_t mismatch on macOS with homebrew apr, unrelated to this patch.

[notroj]

Squash it please.

[SABITHSAHEB]

Done, squashed to a single commit.

[SABITHSAHEB]

Squashed to a single commit and rebased onto current trunk, no content changes.

[notroj]

Analysis by Claude:

The proxy_util.c guard cannot fire, and misses the overflow that is actually there. ap_proxy_canonenc_ex() takes int len, so in ap_assert(len <= (APR_SIZE_MAX - 1) / 3) the left operand is promoted to apr_size_t and compared against ~6.1e18, which is larger than any int — the assertion is always true. A negative len also converts to a large unsigned value and passes. Meanwhile 3 * len + 1 is evaluated in int arithmetic, which is undefined for len > INT_MAX / 3 (~715 million), and that is the case the guard is meant to cover. The check needs to stay in the int domain, e.g. len >= 0 && len <= (INT_MAX - 1) / 3.

The ap_make_full_path() check can wrap in exactly the way it is guarding against. In ap_assert(len1 <= APR_SIZE_MAX - len2 - 3) both len1 and len2 are apr_size_t, so when len2 > APR_SIZE_MAX - 3 the subexpression APR_SIZE_MAX - len2 - 3 wraps to a large value and the assertion passes. len2 needs bounding before it is subtracted.

The remaining guards are not reachable. On 64-bit, 3 * len + 1 overflows only for a string longer than 6.1e18 bytes. The ssl_engine_vars.c cases are bounded far more tightly than that: ciphers_len, extids_len, alpn_len and the rest come from 16-bit TLS extension length fields inside a record capped around 16KB, so * 2 + 1 cannot exceed a few hundred kilobytes. escape_backref()'s path comes from a URI bounded by LimitRequestLine, 8190 by default. None are reachable on 32-bit either, where the threshold is still around 1.4GB.

The first two are worth correcting whatever is decided about the rest, since a guard that cannot fire reads as though the case has been handled.

🤖 Generated with Claude Code

[SABITHSAHEB]

Both points are right. The canonenc check was comparing against the size_t range, so it could never fire, and the make_full_path one could wrap. Both are fixed: canonenc now uses len >= 0 && len <= (INT_MAX - 1) / 3, and make_full_path checks len2 <= APR_SIZE_MAX - 3 before the subtraction.

I agree the others can't be reached, so I dropped them. That covers the ssl_engine_vars, escape_backref, ms_wdv, mod_proxy_ftp and escape_logitem guards, plus the abort() to ap_assert() churn in ap_escape_html2. The PR is now just these two lines, still one commit, and the title and description are updated to match.