Fail on unsupported types instead of terminating connections

neo4j · Apr 2, 2018 · 3e1aac5 · 3e1aac5
1 parent 123524f
commit 3e1aac5
Show file tree

Hide file tree

Showing 12 changed files with 904 additions and 123 deletions.
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/messaging/KnownType.java b/community/bolt/src/main/java/org/neo4j/bolt/messaging/KnownType.java
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2002-2018 "Neo Technology,"
+ * Network Engine for Objects in Lund AB [http://neotechnology.com]
+ *
+ * This file is part of Neo4j.
+ *
+ * Neo4j is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.neo4j.bolt.messaging;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public enum KnownType
+{
+    NODE( 'N', "Node" ),
+    RELATIONSHIP( 'R', "Relationship" ),
+    UNBOUND_RELATIONSHIP( 'r', "Relationship" ),
+    PATH( 'P', "Path" ),
+    POINT_2D( 'X', "Point" ),
+    POINT_3D( 'Y', "Point" ),
+    DATE( 'D', "LocalDate" ),
+    TIME( 'T', "OffsetTime" ),
+    LOCAL_TIME( 't', "LocalTime" ),
+    LOCAL_DATE_TIME( 'd', "LocalDateTime" ),
+    DATE_TIME_WITH_ZONE_OFFSET( 'F', "OffsetDateTime" ),
+    DATE_TIME_WITH_ZONE_NAME( 'f', "ZonedDateTime" ),
+    DURATION( 'E', "Duration" );
+
+    private final byte signature;
+    private final String description;
+
+    KnownType( char signature, String description )
+    {
+        this( (byte)signature, description );
+    }
+
+    KnownType( byte signature, String description )
+    {
+        this.signature = signature;
+        this.description = description;
+    }
+
+    public byte signature()
+    {
+        return signature;
+    }
+
+    public String description()
+    {
+        return description;
+    }
+
+    private static Map<Byte, KnownType> byteToKnownTypeMap = new HashMap<>();
+    static
+    {
+        for ( KnownType type : KnownType.values() )
+        {
+            byteToKnownTypeMap.put( type.signature, type );
+        }
+    }
+
+    public static KnownType valueOf( byte signature )
+    {
+        return byteToKnownTypeMap.get( signature );
+    }
+
+    public static KnownType valueOf( char signature )
+    {
+        return KnownType.valueOf( (byte)signature );
+    }
+}
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/transport/pipeline/MessageAccumulator.java b/community/bolt/src/main/java/org/neo4j/bolt/transport/pipeline/MessageAccumulator.java
@@ -73,10 +73,9 @@ protected void decode( ChannelHandlerContext channelHandlerContext, ByteBuf in,
 
     private void assertNonEmptyMessage()
     {
-        // if we had already set message boundary previously
+        if ( actualReadableBytes() == 0 )
-        if ( readMessageBoundary )
         {
-            throw new DecoderException( "Consecutive message boundaries (empty messages) are not expected." );
+            throw new DecoderException( "Message boundary received when there's nothing to decode." );
         }
     }
 }
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/transport/pipeline/MessageDecoder.java b/community/bolt/src/main/java/org/neo4j/bolt/transport/pipeline/MessageDecoder.java
@@ -24,10 +24,12 @@
 import io.netty.channel.SimpleChannelInboundHandler;
 import io.netty.handler.codec.DecoderException;
 
+import org.neo4j.bolt.v1.messaging.BoltIOException;
 import org.neo4j.bolt.v1.messaging.BoltRequestMessageHandler;
 import org.neo4j.bolt.v1.messaging.BoltRequestMessageReader;
 import org.neo4j.bolt.v1.messaging.Neo4jPack;
 import org.neo4j.bolt.v1.packstream.ByteBufInput;
+import org.neo4j.bolt.v1.runtime.Neo4jError;
 
 import static io.netty.buffer.ByteBufUtil.hexDump;
 
@@ -45,13 +47,24 @@ public MessageDecoder( Neo4jPack pack, BoltRequestMessageHandler messageHandler
     }
 
     @Override
-    protected void channelRead0( ChannelHandlerContext channelHandlerContext, ByteBuf byteBuf )
+    protected void channelRead0( ChannelHandlerContext channelHandlerContext, ByteBuf byteBuf ) throws Exception
     {
         input.start( byteBuf );
         try
         {
             reader.read( messageHandler );
         }
+        catch ( BoltIOException ex )
+        {
+            if ( ex.causesFailure() )
+            {
+                messageHandler.onExternalError( Neo4jError.from( ex ) );
+            }
+            else
+            {
+                throw ex;
+            }
+        }
         catch ( Throwable error )
         {
             throw new DecoderException( "Failed to read inbound message:\n" + hexDump( byteBuf ) + "\n", error );

diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltIOException.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltIOException.java
@@ -43,4 +43,9 @@ public Status status()
     {
         return status;
     }
+
+    public boolean causesFailure()
+    {
+        return status != Status.Request.InvalidFormat;
+    }
 }
diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/BoltRequestMessageReader.java
@@ -23,7 +23,6 @@
 import java.util.Map;
 
 import org.neo4j.bolt.v1.packstream.PackStream;
-import org.neo4j.bolt.v1.runtime.Neo4jError;
 import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.values.virtual.MapValue;
 
@@ -72,15 +71,7 @@ public void read( BoltRequestMessageHandler handler ) throws IOException
                 case RUN:
                     String statement = unpacker.unpackString();
                     MapValue params = unpacker.unpackMap();
-                    Neo4jError error = unpacker.consumeError();
+                    handler.onRun( statement, params );
-                    if ( error != null )
-                    {
-                        handler.onExternalError( error );
-                    }
-                    else
-                    {
-                        handler.onRun( statement, params );
-                    }
                     break;
                 case DISCARD_ALL:
                     handler.onDiscardAll();
@@ -89,20 +80,20 @@ public void read( BoltRequestMessageHandler handler ) throws IOException
                     handler.onPullAll();
                     break;
                 default:
-                    throw new BoltIOException( Status.Request.Invalid,
+                    throw new BoltIOException( Status.Request.InvalidFormat,
-                            "Message 0x" + Integer.toHexString( signature ) + " is not supported." );
+                            String.format( "Message 0x%s is not supported.", Integer.toHexString( signature ) ) );
                 }
             }
             catch ( IllegalArgumentException e )
             {
-                throw new BoltIOException( Status.Request.Invalid,
+                throw new BoltIOException( Status.Request.InvalidFormat,
-                        "0x" + Integer.toHexString( signature ) + " is not a valid message signature." );
+                        String.format( "Message 0x%s is not a valid message signature.", Integer.toHexString( signature ) ) );
             }
         }
         catch ( PackStream.PackStreamException e )
         {
-            throw new BoltIOException( Status.Request.InvalidFormat, "Unable to read message type. " +
+            throw new BoltIOException( Status.Request.InvalidFormat,
-                                                                     "Error was: " + e.getMessage(), e );
+                    String.format( "Unable to read message type. Error was: %s.", e.getMessage() ), e );
         }
     }
 

diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPack.java
@@ -23,7 +23,6 @@
 
 import org.neo4j.bolt.v1.packstream.PackInput;
 import org.neo4j.bolt.v1.packstream.PackOutput;
-import org.neo4j.bolt.v1.runtime.Neo4jError;
 import org.neo4j.values.AnyValue;
 import org.neo4j.values.virtual.MapValue;
 
@@ -61,8 +60,6 @@ interface Unpacker
         char unpackStructSignature() throws IOException;
 
         long unpackListHeader() throws IOException;
-
-        Neo4jError consumeError();
     }
 
     Packer newPacker( PackOutput output );

diff --git a/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPackV1.java b/community/bolt/src/main/java/org/neo4j/bolt/v1/messaging/Neo4jPackV1.java
@@ -30,11 +30,11 @@
 import java.util.List;
 import java.util.Map;
 
+import org.neo4j.bolt.messaging.KnownType;
 import org.neo4j.bolt.v1.packstream.PackInput;
 import org.neo4j.bolt.v1.packstream.PackOutput;
 import org.neo4j.bolt.v1.packstream.PackStream;
 import org.neo4j.bolt.v1.packstream.PackType;
-import org.neo4j.bolt.v1.runtime.Neo4jError;
 import org.neo4j.collection.primitive.PrimitiveLongIntKeyValueArray;
 import org.neo4j.kernel.api.exceptions.Status;
 import org.neo4j.values.AnyValue;
@@ -428,8 +428,6 @@ public void writeByteArray( byte[] value ) throws IOException
 
     protected static class UnpackerV1 extends PackStream.Unpacker implements Neo4jPack.Unpacker
     {
-        private final List<Neo4jError> errors = new ArrayList<>( 2 );
-
         protected UnpackerV1( PackInput input )
         {
             super( input );
@@ -475,8 +473,7 @@ public AnyValue unpack() throws IOException
                 return null;
             }
             default:
-                throw new BoltIOException( Status.Request.InvalidFormat,
+                throw new BoltIOException( Status.Request.InvalidFormat, "Unknown value type: " + valType );
-                        "Unknown value type: " + valType );
             }
         }
 
@@ -519,28 +516,15 @@ else if ( size == UNKNOWN_SIZE )
 
         protected AnyValue unpackStruct( char signature ) throws IOException
         {
-            switch ( signature )
+            KnownType knownType = KnownType.valueOf( signature );
-            {
+            if ( knownType == null )
-            case NODE:
             {
-                throw new BoltIOException( Status.Request.Invalid, "Nodes cannot be unpacked." );
-            }
-            case RELATIONSHIP:
-            {
-                throw new BoltIOException( Status.Request.Invalid, "Relationships cannot be unpacked." );
-            }
-            case UNBOUND_RELATIONSHIP:
-            {
-                throw new BoltIOException( Status.Request.Invalid, "Relationships cannot be unpacked." );
-            }
-            case PATH:
-            {
-                throw new BoltIOException( Status.Request.Invalid, "Paths cannot be unpacked." );
-            }
-            default:
                 throw new BoltIOException( Status.Request.InvalidFormat,
-                        "Unknown struct type: " + Integer.toHexString( signature ) );
+                        String.format( "Struct types of 0x%s are not recognized.", Integer.toHexString( signature ) ) );
             }
+
+            throw new BoltIOException( Status.Statement.TypeError,
+                    String.format( "%s values cannot be unpacked with this version of bolt.", knownType.description() ) );
         }
 
         @Override
@@ -572,19 +556,13 @@ public MapValue unpackMap() throws IOException
                         val = unpack();
                         if ( map.put( key, val ) != null )
                         {
-                            errors.add(
+                            throw new BoltIOException( Status.Request.Invalid, "Duplicate map key `" + key + "`." );
-                                    Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ) );
                         }
                         break;
                     case NULL:
-                        errors.add( Neo4jError.from( Status.Request.Invalid,
+                        throw new BoltIOException( Status.Request.Invalid, "Value `null` is not supported as key in maps, must be a non-nullable string." );
-                                "Value `null` is not supported as key in maps, must be a non-nullable string." ) );
-                        unpackNull();
-                        val = unpack();
-                        map.put( null, val );
-                        break;
                     default:
-                        throw new PackStream.PackStreamException( "Bad key type" );
+                        throw new BoltIOException( Status.Request.InvalidFormat, "Bad key type: " + keyType );
                     }
                 }
             }
@@ -593,39 +571,27 @@ public MapValue unpackMap() throws IOException
                 map = new HashMap<>( size, 1 );
                 for ( int i = 0; i < size; i++ )
                 {
-                    PackType type = peekNextType();
+                    PackType keyType = peekNextType();
                     String key;
-                    switch ( type )
+                    switch ( keyType )
                     {
                     case NULL:
-                        errors.add( Neo4jError.from( Status.Request.Invalid,
+                        throw new BoltIOException( Status.Request.Invalid, "Value `null` is not supported as key in maps, must be a non-nullable string." );
-                                "Value `null` is not supported as key in maps, must be a non-nullable string." ) );
-                        unpackNull();
-                        key = null;
-                        break;
                     case STRING:
                         key = unpackString();
                         break;
                     default:
-                        throw new PackStream.PackStreamException( "Bad key type: " + type );
+                        throw new BoltIOException( Status.Request.InvalidFormat, "Bad key type: " + keyType );
                     }
 
                     AnyValue val = unpack();
                     if ( map.put( key, val ) != null )
                     {
-                        errors.add( Neo4jError.from( Status.Request.Invalid, "Duplicate map key `" + key + "`." ) );
+                        throw new BoltIOException( Status.Request.Invalid, "Duplicate map key `" + key + "`." );
                     }
                 }
             }
             return VirtualValues.map( map );
         }
-
-        @Override
-        public Neo4jError consumeError()
-        {
-            Neo4jError error = Neo4jError.combine( errors );
-            errors.clear();
-            return error;
-        }
     }
 }