Skip to content

fix char-signedness out-of-bounds read in url and header lookup tables#3376

Open
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:char-signedness-oob
Open

fix char-signedness out-of-bounds read in url and header lookup tables#3376
ubeddulla wants to merge 1 commit into
apache:masterfrom
ubeddulla:char-signedness-oob

Conversation

@ubeddulla

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Two +128-biased 256-entry lookup tables are read out of bounds when an input byte has the high bit set, on builds where char is unsigned (aarch64, riscv64).

  1. g_url_parsing_fast_action_map (uri.cpp, used by URI::SetHttpURL and ParseURL) is indexed with (int)*p, where *p walks the untrusted request-URI. On an unsigned-char platform a byte in 0x80..0xFF gives index 128..255, so raw[256..383] is read, up to 127 bytes past the table.
  2. g_tolower_map (ascii_tolower in case_ignored_flat_map.h) has the same +128 bias and is indexed by the promoted char argument. It is reached from CaseIgnoredHasher over HTTP header names and from HPACK, so the same high-bit byte over-reads the 256-entry table. The note already there records an aarch64 char-signedness crash; making the parameter int did not fix it, because a char argument still promotes to 0..255.

On x86_64 (char signed) both indices fall in [-128,127] and stay in bounds, which is why the current CI does not surface it.

What is changed and the side effects?

Changed:

Fold the index back into signed-char range at the three lookup sites with a (signed char) cast. The table layout is untouched and behavior on signed-char platforms is unchanged. Verified with an ASan build under -funsigned-char: a URL host byte 0xC3 and a header-name byte >= 0x80 each trip a buffer-overflow before the change and run clean after, and ascii_tolower output matches the expected value for all 256 byte values.

Side effects:

  • Performance effects: none (a single cast).

  • Breaking backward compatibility: no.


Check List:

@wwbmmm

wwbmmm commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@ubeddulla Thank you for your PR! Could you add some test cases?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes out-of-bounds reads caused by char signedness when indexing +128-biased 256-entry lookup tables used in URL parsing and ASCII lowercasing (affecting unsigned-char platforms like aarch64/riscv64).

Changes:

  • Casts URL parsing table indices to (signed char) to keep indexing within [-128, 127] in URI::SetHttpURL and ParseURL.
  • Casts ascii_tolower’s index to (signed char) to prevent OOB reads when header-name bytes are >= 0x80.
  • Adds clarifying comments explaining the signedness/bias assumption for the tolower table.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/butil/containers/case_ignored_flat_map.h Prevents OOB reads in ascii_tolower by forcing signed-char indexing into the biased table.
src/brpc/uri.cpp Prevents OOB reads in URL parsing by forcing signed-char indexing into the biased action table.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +32 to +36
// g_tolower_map is biased by +128 and sized for a signed-char index
// ([-128,127]). Callers pass a `char`; on platforms where `char` is
// unsigned (aarch64, riscv64) a byte >= 0x80 arrives here as 128..255 and
// indexes past the 256-entry table. Fold back into signed-char range.
return g_tolower_map[(signed char)c];
Comment thread src/brpc/uri.cpp
Comment on lines 168 to 172
bool need_scheme = true;
bool need_user_info = true;
for (; true; ++p) {
const char action = g_url_parsing_fast_action_map[(int)*p];
const char action = g_url_parsing_fast_action_map[(signed char)*p];
if (action == URI_PARSE_CONTINUE) {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants