Repository: netty/netty
Issue: MQTT unknown message type isn't handled as decoding error
Labels: defect

### Expected behavior

When there is an MQTT decoding error, an invalidMessage is created and a DecodeResult with failure and related cause exception is filled with right information so that the user application can see what's the problem. It should happens in any step in the decoding phase.

### Actual behavior

It doesn't work if the problem is in the fixed header with a not valid MQTT message type.
Poisoning a message with the first byte set to 0xF0 (so 15 that is not a valid MQTT message type), the IllegalArgumentException is raised and propagated. There is no way to have a failed DecodeResult as in the other scenarios.

### Steps to reproduce

Building a server pipeline with the MqttDecoder and then after opening the connection sending a packet with just two bytes 0xF0 0x00.

### Netty version

4.1.13 Final

### JVM version (e.g. `java -version`)

### OS version (e.g. `uname -a`)


Issue comments:
Fixed by https://github.com/netty/netty/pull/6985

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
index c8a3546e0ed..8d94ee48e96 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
@@ -71,11 +71,15 @@ public MqttDecoder(int maxBytesInMessage) {
     @Override
     protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> out) throws Exception {
         switch (state()) {
-            case READ_FIXED_HEADER:
+            case READ_FIXED_HEADER: try {
                 mqttFixedHeader = decodeFixedHeader(buffer);
                 bytesRemainingInVariablePart = mqttFixedHeader.remainingLength();
                 checkpoint(DecoderState.READ_VARIABLE_HEADER);
                 // fall through
+            } catch (Exception cause) {
+                out.add(invalidMessage(cause));
+                return;
+            }
 
             case READ_VARIABLE_HEADER:  try {
                 if (bytesRemainingInVariablePart > maxBytesInMessage) {


Test patch:
diff --git a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
index 21a69055ed5..dd6c74269fa 100644
--- a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
+++ b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
@@ -239,6 +239,29 @@ public void testDisconnectMessage() throws Exception {
         testMessageWithOnlyFixedHeader(MqttMessageType.DISCONNECT);
     }
 
+    @Test
+    public void testUnknownMessageType() throws Exception {
+
+        final MqttMessage message = createMessageWithFixedHeader(MqttMessageType.PINGREQ);
+        ByteBuf byteBuf = MqttEncoder.doEncode(ALLOCATOR, message);
+        try {
+            // setting an invalid message type (15, reserved and forbidden by MQTT 3.1.1 spec)
+            byteBuf.setByte(0, 0xF0);
+            final List<Object> out = new LinkedList<Object>();
+            mqttDecoder.decode(ctx, byteBuf, out);
+
+            assertEquals("Expected one object but got " + out.size(), 1, out.size());
+
+            final MqttMessage decodedMessage = (MqttMessage) out.get(0);
+            assertTrue(decodedMessage.decoderResult().isFailure());
+            Throwable cause = decodedMessage.decoderResult().cause();
+            assertTrue(cause instanceof IllegalArgumentException);
+            assertEquals("unknown message type: 15", cause.getMessage());
+        } finally {
+            byteBuf.release();
+        }
+    }
+
     private void testMessageWithOnlyFixedHeader(MqttMessageType messageType) throws Exception {
         MqttMessage message = createMessageWithFixedHeader(messageType);
         ByteBuf byteBuf = MqttEncoder.doEncode(ALLOCATOR, message);