[BUG] UTF8Util.isWellFormed inverts the surrogate-pair check: valid non-BMP characters are rejected, lone surrogates accepted

[ImDanXie]

Environment

• BiFroMQ 4.0.0
• Verified against main @ ba2c34e0 — still present on the latest main at the time of filing
• Module: bifromq-util
• File: bifromq-util/src/main/java/org/apache/bifromq/util/UTF8Util.java:45-57

Summary

Character.isSurrogatePair(high, low) returns true when the two chars form a valid UTF-16 surrogate pair. The code treats that as a validation failure, inverting the check. As a result:

• well-formed non-BMP characters (emoji, CJK Extension B and above) are rejected, and
• unpaired/lone surrogates — the sequence the spec actually forbids — are accepted.

Code

for (int i = 1; i < len; i++) {
    final char cr = str.charAt(i);
    if (cr == '\u0000') {
        return false;
    }
    if (Character.isSurrogatePair(cl, cr)) {   // true only for a VALID pair
        return false;                          // <-- rejects legal input
    }
    if (sanityCheck && isUnacceptableChar(cr)) {
        return false;
    }
    cl = cr;
}

Evidence

UTF8Util.isWellFormed("😀", false);              // U+1F600, a valid pair -> returns FALSE
UTF8Util.isWellFormed("a\uD83Db", false);        // lone high surrogate   -> returns TRUE

The first is a well-formed encoding of U+1F600; the second is malformed UTF-16 that the spec forbids.

Running these against the compiled bifromq-util classes on main @ ba2c34e0 gives exactly the above. A control with BMP text ("中文") correctly returns true, confirming the defect is specific to non-BMP handling.

Specification reference

• MQTT 5.0 [MQTT-1.5.4-1] and MQTT 3.1.1 [MQTT-1.5.3-1]: character data MUST be well-formed UTF-8 as defined by the Unicode specification / RFC 3629, and MUST NOT include encodings of code points between U+D800 and U+DFFF.

Non-BMP code points are encoded as 4-byte UTF-8 sequences and are therefore legal. Only the surrogate code points themselves are forbidden.

Impact

isWellFormed guards the following fields:

Field	Call site
ClientId	MQTT3ConnectHandler.java:124, MQTT5ConnectHandler.java:155
UserName	MQTT3ConnectHandler.java:135, MQTT5ConnectHandler.java:186
Will topic	MQTT3ConnectHandler.java:139, MQTT5ConnectHandler.java:210
Topic filter	MQTT5ProtocolHelper.java:250, :360
Topic	MQTT5ProtocolHelper.java:539
Response topic	MQTT5ProtocolHelper.java:589

Any client using an emoji or a CJK Extension B+ character in one of these fields is rejected as a protocol error and disconnected — an interoperability defect against both MQTT versions. (Common BMP CJK ideographs, U+4E00–U+9FFF, are unaffected; the impact is emoji and Extension B+ rare/ideographic characters, which occur in personal and place names.)

Note that the sibling validator isValidUTF8Payload, which goes through CharsetDecoder, handles lone surrogates correctly — the two validation paths disagree, which is further evidence that the logic above is inverted.

Why existing tests do not catch it

bifromq-util/src/test/java/org/apache/bifromq/util/UTF8UtilTest.java:50 asserts the incorrect behavior as expected:

assertFalse(UTF8Util.isWellFormed("hello😊world", false)); // surrogate pairs

😊 is a valid encoding of U+1F60A, so this assertion should read assertTrue. The test must be corrected together with the fix.

Suggested fix

Validate for unpaired surrogates instead, keeping the existing structure (every character is still checked for sanityCheck, as before):

char cl = str.charAt(0);
if (cl == '\u0000') {
    return false;
}
if (Character.isLowSurrogate(cl)) {
    return false;                       // unpaired low surrogate
}
if (sanityCheck && isUnacceptableChar(cl)) {
    return false;
}
for (int i = 1; i < len; i++) {
    final char cr = str.charAt(i);
    if (cr == '\u0000') {
        return false;
    }
    if (Character.isHighSurrogate(cl)) {
        if (!Character.isLowSurrogate(cr)) {
            return false;               // unpaired high surrogate
        }
    } else if (Character.isLowSurrogate(cr)) {
        return false;                   // unpaired low surrogate
    }
    if (sanityCheck && isUnacceptableChar(cr)) {
        return false;
    }
    cl = cr;
}
return !Character.isHighSurrogate(cl);  // trailing lone high surrogate

Note the trailing check after the loop: a high surrogate as the last character ("ab\uD83D") would otherwise pass, because cl is only validated when a following character exists. (Character.isDefined returns true for surrogate code points, so isUnacceptableChar never catches them — the pairing must be handled explicitly.)