From dd026eb60a68285d5d89b1fbf7ea3024f47632fd Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Sat, 19 Jul 2014 21:51:19 +0400 Subject: [PATCH] Fix NPE problems Motivation: Now Netty has a few problems with null values. Modifications: - Check File in DiskFileUpload.toString(). If File is null we will get NPE when calling toString() method. - Check Result in MqttDecoder.decodeConnectionPayload(...). - Check Unsafe before calling unsafe.getClass() in PlatformDependent0 static block. - Removed unnecessary null check in WebSocket08FrameEncoder.encode(...). Because msg.content() can not return null. - Removed unnecessary null checks in ConcurrentHashMapV8.removeTreeNode(TreeNode). - Removed unnecessary null check in OioDatagramChannel.doReadMessages(List). Because tmpPacket.getSocketAddress() always returns new SocketAddress instance. - Removed unnecessary null check in OioServerSocketChannel.doReadMessages(List). Because socket.accept() always returns new Socket instance. - Pass Unpooled.buffer(0) instead of null inside CloseWebSocketFrame(boolean, int) constructor. If we will pass null we will get NPE in super class constructor. - Added throw new IllegalStateException in GlobalEventExecutor.awaitInactivity(long, TimeUnit) if it will be called before GlobalEventExecutor.execute(Runnable). Because now we will get NPE. IllegalStateException will be better in this case. - Fixed null check in OpenSslServerContext.setTicketKeys(byte[]). Now we throw new NPE if byte[] is not null. Result: Added new null checks when it is necessary, removed unnecessary null checks and fixed some NPE problems. --- .../codec/http/multipart/DiskFileUpload.java | 2 +- .../http/websocketx/CloseWebSocketFrame.java | 2 +- .../http/websocketx/WebSocket08FrameEncoder.java | 8 +------- .../util/concurrent/GlobalEventExecutor.java | 7 ++++--- .../netty/util/internal/PlatformDependent0.java | 11 ++++++----- .../util/internal/chmv8/ConcurrentHashMapV8.java | 8 ++++---- .../netty/handler/ssl/OpenSslServerContext.java | 2 +- .../channel/socket/oio/OioDatagramChannel.java | 3 --- .../socket/oio/OioServerSocketChannel.java | 16 ++++++---------- 9 files changed, 24 insertions(+), 35 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java index 69cb190deb..5d2186c80d 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java @@ -133,7 +133,7 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload { HttpHeaders.Names.CONTENT_LENGTH + ": " + length() + "\r\n" + "Completed: " + isCompleted() + "\r\nIsInMemory: " + isInMemory() + "\r\nRealFile: " + - file.getAbsolutePath() + " DefaultDeleteAfter: " + + (file != null ? file.getAbsolutePath() : "null") + " DefaultDeleteAfter: " + deleteOnExitTemporaryFile; } diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java index 8d6c56b2cd..ccbd89c0f6 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/CloseWebSocketFrame.java @@ -54,7 +54,7 @@ public class CloseWebSocketFrame extends WebSocketFrame { * reserved bits used for protocol extensions */ public CloseWebSocketFrame(boolean finalFragment, int rsv) { - this(finalFragment, rsv, null); + this(finalFragment, rsv, Unpooled.buffer(0)); } /** diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket08FrameEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket08FrameEncoder.java index bdad55b928..1589a97502 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket08FrameEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket08FrameEncoder.java @@ -54,7 +54,6 @@ package io.netty.handler.codec.http.websocketx; import io.netty.buffer.ByteBuf; -import io.netty.buffer.Unpooled; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.MessageToMessageEncoder; import io.netty.handler.codec.TooLongFrameException; @@ -96,14 +95,9 @@ public class WebSocket08FrameEncoder extends MessageToMessageEncoder out) throws Exception { - + final ByteBuf data = msg.content(); byte[] mask; - ByteBuf data = msg.content(); - if (data == null) { - data = Unpooled.EMPTY_BUFFER; - } - byte opcode; if (msg instanceof TextWebSocketFrame) { opcode = OPCODE_TEXT; diff --git a/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java b/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java index db7a5599fb..0a546c7413 100644 --- a/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java +++ b/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java @@ -203,10 +203,11 @@ public final class GlobalEventExecutor extends AbstractEventExecutor { throw new NullPointerException("unit"); } - Thread thread = this.thread; - if (thread != null) { - thread.join(unit.toMillis(timeout)); + final Thread thread = this.thread; + if (thread == null) { + throw new IllegalStateException("thread was not started"); } + thread.join(unit.toMillis(timeout)); return !thread.isAlive(); } diff --git a/common/src/main/java/io/netty/util/internal/PlatformDependent0.java b/common/src/main/java/io/netty/util/internal/PlatformDependent0.java index f58be03d66..39414f3c6f 100644 --- a/common/src/main/java/io/netty/util/internal/PlatformDependent0.java +++ b/common/src/main/java/io/netty/util/internal/PlatformDependent0.java @@ -80,16 +80,17 @@ final class PlatformDependent0 { Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe"); unsafeField.setAccessible(true); unsafe = (Unsafe) unsafeField.get(null); - logger.debug("sun.misc.Unsafe.theUnsafe: {}", unsafe != null? "available" : "unavailable"); + logger.debug("sun.misc.Unsafe.theUnsafe: {}", unsafe != null ? "available" : "unavailable"); // Ensure the unsafe supports all necessary methods to work around the mistake in the latest OpenJDK. // https://github.com/netty/netty/issues/1061 // http://www.mail-archive.com/jdk6-dev@openjdk.java.net/msg00698.html try { - unsafe.getClass().getDeclaredMethod( - "copyMemory", Object.class, long.class, Object.class, long.class, long.class); - - logger.debug("sun.misc.Unsafe.copyMemory: available"); + if (unsafe != null) { + unsafe.getClass().getDeclaredMethod( + "copyMemory", Object.class, long.class, Object.class, long.class, long.class); + logger.debug("sun.misc.Unsafe.copyMemory: available"); + } } catch (NoSuchMethodError t) { logger.debug("sun.misc.Unsafe.copyMemory: unavailable"); throw t; diff --git a/common/src/main/java/io/netty/util/internal/chmv8/ConcurrentHashMapV8.java b/common/src/main/java/io/netty/util/internal/chmv8/ConcurrentHashMapV8.java index fa69fc55db..72c2705f31 100644 --- a/common/src/main/java/io/netty/util/internal/chmv8/ConcurrentHashMapV8.java +++ b/common/src/main/java/io/netty/util/internal/chmv8/ConcurrentHashMapV8.java @@ -2830,14 +2830,14 @@ public class ConcurrentHashMapV8 else sp.right = p; } - if ((s.right = pr) != null) - pr.parent = s; + s.right = pr; + pr.parent = s; } p.left = null; + s.left = pl; + pl.parent = s; if ((p.right = sr) != null) sr.parent = p; - if ((s.left = pl) != null) - pl.parent = s; if ((s.parent = pp) == null) r = s; else if (p == pp.left) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java index b1a0257a15..a62c6d7b2d 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java @@ -322,7 +322,7 @@ public final class OpenSslServerContext extends SslContext { * Sets the SSL session ticket keys of this context. */ public void setTicketKeys(byte[] keys) { - if (keys != null) { + if (keys == null) { throw new NullPointerException("keys"); } SSLContext.setSessionTicketKeys(ctx, keys); diff --git a/transport/src/main/java/io/netty/channel/socket/oio/OioDatagramChannel.java b/transport/src/main/java/io/netty/channel/socket/oio/OioDatagramChannel.java index eeab486184..f8e96e6e84 100644 --- a/transport/src/main/java/io/netty/channel/socket/oio/OioDatagramChannel.java +++ b/transport/src/main/java/io/netty/channel/socket/oio/OioDatagramChannel.java @@ -209,9 +209,6 @@ public class OioDatagramChannel extends AbstractOioMessageChannel socket.receive(tmpPacket); InetSocketAddress remoteAddr = (InetSocketAddress) tmpPacket.getSocketAddress(); - if (remoteAddr == null) { - remoteAddr = remoteAddress(); - } int readBytes = tmpPacket.getLength(); allocHandle.record(readBytes); diff --git a/transport/src/main/java/io/netty/channel/socket/oio/OioServerSocketChannel.java b/transport/src/main/java/io/netty/channel/socket/oio/OioServerSocketChannel.java index 72ce74c521..ed2e019f0e 100644 --- a/transport/src/main/java/io/netty/channel/socket/oio/OioServerSocketChannel.java +++ b/transport/src/main/java/io/netty/channel/socket/oio/OioServerSocketChannel.java @@ -153,18 +153,14 @@ public class OioServerSocketChannel extends AbstractOioMessageChannel try { Socket s = socket.accept(); try { - if (s != null) { - buf.add(new OioSocketChannel(this, s)); - return 1; - } + buf.add(new OioSocketChannel(this, s)); + return 1; } catch (Throwable t) { logger.warn("Failed to create a new channel from an accepted socket.", t); - if (s != null) { - try { - s.close(); - } catch (Throwable t2) { - logger.warn("Failed to close a socket.", t2); - } + try { + s.close(); + } catch (Throwable t2) { + logger.warn("Failed to close a socket.", t2); } } } catch (SocketTimeoutException e) {