Path forward for full UTF-8 correctness

[mattrpav]

Background

As reported here: #2644 and spot fixed previously, ActiveMQ currently has incomplete coverage of the entire UTF-8 range when working with text-bodied messages.

Challenges with fixing it

1. Fixing it would mean potentially breaking existing users that have work arounds in place.

2. All clients and all brokers in the message flow path must be aligned or there is potential for issues. There is no iterative rollout option and rollback is everyone all at once.

3. Across protocol impacts: Non-OpenWire clients may be coded to work-around (AMQP, STOMP, etc)

Approaches

A couple fix options:

1. Use a env/sys property to enable proper UTF-8 rendering. Have a static code block that detects and then sets a flag to route desired behavior

2. Force fixed behavior as part of OpenWire v13

3. Make the ‘fixed’ behavior the default in ActiveMQ 7 and/or a combination of ActiveMQ 7 and OpenWire v13

4. Offer a env/sys flag to rollback to previously ‘incorrect’ behavior

5. Add an 'upgrade check' certification tool to analyze existing message stores for messages relying on modified UTF-8.

Motivation

Usage of Emoji (and potentially other uses of UTF-8 character range) will only increase across applications.

[jbonofre]

Thanks Matt!

I think the options combine rather than compete. I suggest this path:

1. Tie the fix to OpenWire v13 negotiation. This is the only approach that addresses the "all clients and brokers must align" constraint. WireFormatInfo already negotiates the version, so a v13 connection would use correct UTF-8 and older connections would keep the legacy encoding.
2. Make it the default in ActiveMQ 7. Changing the bytes on the wire can break users who have workarounds, so it should be in a major version (with a Release Note 😄 ).
3. We should probably keep a rollback flag for least one major version. It's cheap, and it gives workaround users an escape hatch.
4. I think a JVM-wide static flag brings back the "everyone at once, rollback is everyone at once" problem, so I would not make it the primary mechanism.

It opens more questions:

• The messages already stored (KahaDB, JDBC, ...) were written with the legacy encoding. We need to decide whether the stores keep the original bytes or re-marshal, and make sure old data is still readable after the switch.
• AMQP and STOMP presumably already use standard UTF-8. We should test bridging in both directions, so that clients with workarounds don't see double-encoded data.
• A log warning or metric when the legacy encoder sees a supplementary character, would let operators find out whether they are affected before the default changes.
• NMS and the others would need a matching change keyed off the same version negotiation.

[mattrpav]

h/t @cshannon on Slack

• Consider way to detect the modified UTF-8 format for existing messages on disk and retain behavior

[cshannon]

There is really no way to fix this without breaking the ability to read existing messages. OpenWire version doesn't help, it's not related to that because it's not tied to the OpenWire version per se. Each message that is persisted could have the previous encoding.

To "fix" this would require starting fresh with an empty store. The other option is the journal compaction code that I'm working on could be leveraged to rewrite messages with the new encoding as an offline task.

[cshannon]

Also, keep in mind different protocols don't help here because the data is stored using OpenWire, so even if AMQP is used it gets changed during the conversion to store.

[cshannon]

I thought this issue sounded familiar and its because it is. I already fixed it for Stomp #1290

The same changes probably to be fixed for AMQP based on #2644

We can still look at fixing it long term by using a non modified UTF-8 in a major version, but that fix would at least fix AMQP (assuming it's the same conversion issue that STOMP had)

[cshannon]

Here's the original jira issue for STOMP https://issues.apache.org/jira/browse/AMQ-8398

[artnaseef]

My 2 cents - unless there is a problem in the internals of ActiveMQ that create inconsistency there, a solution closer to the clients will keep the complexity of any fix here down.

Making a change to client libraries, or even the client connection handling in the broker (ideally with an optional connection setting or similar to choose), can isolate the problem and fix well.

Another way to word this: convert the text into the broker's internal standard as early as possible, and convert it back out as late as possible.

Also, didn't the initial reporter mention it worked right in one version and then broke in another? If so, what changed?

Hope this helps.

[cshannon]

What was changed was already linked: #2644 (comment) which fixed a race condition.

As I already stated, the issue is already know and we know how to fix it. The fix is pretty much do the same thing that was done for Stomp in #1290 for the AMQP protocol. conversions need to be done when messages are received/sent from other clients that use standard UTF-8 vs the modified UTF-8

[livicitu]

One more case for this discussion: an OpenWire client that writes standard UTF-8 itself.

Not every OpenWire client is Java.
ActiveMQ-CPP writes a TextMessage body as the bytes the application gives it, so text with a supplementary character arrives as a standard 4-byte UTF-8 sequence, which modified UTF-8 never contains.

• On Classic the broker stores those bytes as they are, and a Java consumer fails in ActiveMQTextMessage.getText(), in MarshallingSupport.readUTF8, with a UTFDataFormatException.
• On Artemis the converter decodes the text on arrival with the same readUTF8, so the send itself is rejected (ARTEMIS-6304, closed so that Artemis can inherit the fix made here).

This direction may be the easiest part to fix without breaking anyone: readUTF8 could accept a well-formed 4-byte UTF-8 sequence and decode it to its surrogate pair.
No valid modified UTF-8 contains such a sequence (lead bytes 0xF0 to 0xF4), so every message that decodes today decodes the same way, nothing on disk needs rewriting, and no version negotiation is needed.
It would also cover clients that can never negotiate a new OpenWire version, such as ActiveMQ-CPP.

We will convert to modified UTF-8 on our side regardless.
This is about the broker and the Java client accepting what non-Java OpenWire clients already send.

[mattrpav]

Interesting. I like the approach of fixing the read side, esp if it doesn't break existing messages on disk.

[cshannon]

@livicitu - That is a good idea, if we can handle it on the read side and detect the format but I'm not sure that works in all cases because of legacy clients.

I think everyone agrees that the ultimate goal should just be using standard UTF-8 everywhere, but the hold up is just backwards compatibility. If we can fallback and detect the old legacy modified format transparently and convert that makes it easier as things won't break if here is the old format and no conversion needed. This would let the broker handle both java openwire and current encoding as well as other clients like stomp/amqp that send standard UTF-8 (or even as you noted other openwire clients)

However, older java clients would potentially break right? They would not be able to always decode standard UTF-8 byte case without an update.

There's also still the problem of consumers for other protocols. If readUTF8() can handle both formats, then a stomp or AMQP producer doesn't need conversion and the bytes can be stored as is. But we still need to convert for a stomp/amqp consumer because we might have the modified format that they wouldn't be able to read.

[cshannon]

Thinking about this more, I don't think that doing the conversion there is a good idea because scanning each byte to detect and convert would likely be very slow.

I think the only real path forward would be to new openwire version and kahadb version, etc so we don't have to always check and convert.

[mattrpav]

@cshannon I think you are probably on the right path here. Protocol conversion is already a throughout/latency hotspot. This would also reduce risk and allow for a clean and certain "moving forward it is fix" approach.

Perhaps a we could add a KahaDB conversion tool to look for invalid UTF-8 messages on older stores to warn users as part of an 'upgrade check' certification step.

[livicitu]

@cshannon thanks. Two clarifications, because I think the read-side change is smaller than it sounds.

It adds no scan.
convertUTF8WithBuf already decodes byte by byte, choosing a 1-, 2- or 3-byte form from each lead byte, and a 4-byte lead (0xF0 to 0xF4) falls into the final else and throws:

activemq/activemq-client/src/main/java/org/apache/activemq/util/MarshallingSupport.java

Lines 418 to 438 in 6475354

	while (count < utfSize) {
	if ((out[s] = (char) buf[offset + count++]) < '\u0080')
	s++;
	else if (((a = out[s]) & 0xe0) == 0xc0) {
	if (count >= utfSize)
	throw new UTFDataFormatException();
	int b = buf[offset + count++];
	if ((b & 0xC0) != 0x80)
	throw new UTFDataFormatException();
	out[s++] = (char) (((a & 0x1F) << 6) | (b & 0x3F));
	} else if ((a & 0xf0) == 0xe0) {
	if (count + 1 >= utfSize)
	throw new UTFDataFormatException();
	int b = buf[offset + count++];
	int c = buf[offset + count++];
	if (((b & 0xC0) != 0x80) || ((c & 0xC0) != 0x80))
	throw new UTFDataFormatException();
	out[s++] = (char) (((a & 0x0F) << 12) | ((b & 0x3F) << 6) | (c & 0x3F));
	} else {
	throw new UTFDataFormatException();
	}

The change is one more branch at that else, decoding the four bytes to a surrogate pair.
A message that decodes today never reaches it, and two chars from four bytes still fit the existing char[utflen].

It makes no client worse off.
An older Java consumer only meets standard 4-byte UTF-8 when a producer such as ActiveMQ-CPP already sent it, and it already fails on that message today.
Updated clients and brokers would read it; everything else behaves exactly as now.

Agreed that it does not replace the conversion STOMP and AMQP consumers need, or the longer-term move to standard UTF-8 with new OpenWire and KahaDB versions.
It is a small step that could go into a minor release, since no input that works today decodes differently.

[cshannon]

@livicitu - Thanks for the clarification, I am following now, I was reading that it would require a second pass. Feel free to take a shot at a PR if you want to (or have time), else one of us can get to it. We will need to work on a plan to fix this incrementally as you mentioned.

[livicitu]

Thanks. I've opened #2698 with the read-side change and tests.

[cshannon]

Thanks for doing that, we are about to do releases now so it will likely go into the next release cycle.