From c632af7736cb58de35b0be5cf348302892b2b40a Mon Sep 17 00:00:00 2001 From: Mark Rotteveel Date: Sat, 6 Jan 2024 14:02:39 +0100 Subject: [PATCH] #778 Potential NPE when warningMessageCallback is null --- src/docs/asciidoc/release_notes.adoc | 4 ++++ .../gds/ng/wire/AbstractWireOperations.java | 17 ++++++++++------- .../gds/ng/wire/ProtocolDescriptor.java | 4 ++-- .../firebirdsql/gds/ng/wire/WireConnection.java | 6 ++++-- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/docs/asciidoc/release_notes.adoc b/src/docs/asciidoc/release_notes.adoc index dc9ca26b9..ba925f1aa 100644 --- a/src/docs/asciidoc/release_notes.adoc +++ b/src/docs/asciidoc/release_notes.adoc @@ -32,6 +32,10 @@ For known issues, consult <>. The following has been changed or fixed since Jaybird 5.0.3: +* Fixed: Potential NPE when `warningMessageCallback` is `null` while reading operations or consuming packets (https://github.com/FirebirdSQL/jaybird/issues/778[#778]) ++ +As part of this change, the constructor `AbstractWireOperations` now explicitly requires the `connection` and `defaultWarningMessageCallback` to be non-``null`` and throws a `NullPointerException` otherwise. +This may affect custom protocol implementations extending `AbstractWireOperations` and/or calling `ProtocolDescriptor#createWireOperations(WireConnection, WarningMessageCallback)`: make sure you don't pass `null`. * ... [#jaybird-5-0-3-changelog] diff --git a/src/main/org/firebirdsql/gds/ng/wire/AbstractWireOperations.java b/src/main/org/firebirdsql/gds/ng/wire/AbstractWireOperations.java index 447c8c258..838e912b3 100644 --- a/src/main/org/firebirdsql/gds/ng/wire/AbstractWireOperations.java +++ b/src/main/org/firebirdsql/gds/ng/wire/AbstractWireOperations.java @@ -39,6 +39,7 @@ import java.sql.SQLWarning; import java.util.List; +import static java.util.Objects.requireNonNull; import static org.firebirdsql.gds.ISCConstants.*; import static org.firebirdsql.gds.impl.wire.WireProtocolConstants.*; @@ -55,8 +56,9 @@ public abstract class AbstractWireOperations implements FbWireOperations { protected AbstractWireOperations(WireConnection connection, WarningMessageCallback defaultWarningMessageCallback) { - this.connection = connection; - this.defaultWarningMessageCallback = defaultWarningMessageCallback; + this.connection = requireNonNull(connection, "connection"); + this.defaultWarningMessageCallback = + requireNonNull(defaultWarningMessageCallback, "defaultWarningMessageCallback"); } @Override @@ -262,18 +264,19 @@ public final void processResponse(Response response) throws SQLException { * Response to process */ public final void processResponseWarnings(final Response response, WarningMessageCallback warningCallback) { - if (warningCallback == null) { - warningCallback = defaultWarningMessageCallback; - } if (response instanceof GenericResponse) { GenericResponse genericResponse = (GenericResponse) response; SQLException exception = genericResponse.getException(); if (exception instanceof SQLWarning) { - warningCallback.processWarning((SQLWarning) exception); + orDefault(warningCallback).processWarning((SQLWarning) exception); } } } + private WarningMessageCallback orDefault(WarningMessageCallback warningMessageCallback) { + return warningMessageCallback != null ? warningMessageCallback : defaultWarningMessageCallback; + } + @Override public final GenericResponse readGenericResponse(WarningMessageCallback warningCallback) throws SQLException, IOException { @@ -297,7 +300,7 @@ public final void consumePackets(int numberOfResponses, WarningMessageCallback w try { readResponse(warningCallback); } catch (Exception e) { - warningCallback.processWarning(new SQLWarning(e)); + orDefault(warningCallback).processWarning(new SQLWarning(e)); // ignoring exceptions log.warnDebug("Exception in consumePackets", e); } diff --git a/src/main/org/firebirdsql/gds/ng/wire/ProtocolDescriptor.java b/src/main/org/firebirdsql/gds/ng/wire/ProtocolDescriptor.java index d51c8b05c..395fb11ea 100644 --- a/src/main/org/firebirdsql/gds/ng/wire/ProtocolDescriptor.java +++ b/src/main/org/firebirdsql/gds/ng/wire/ProtocolDescriptor.java @@ -214,9 +214,9 @@ FbWireBlob createInputBlob(FbWireDatabase database, FbWireTransaction transactio * Create an {@link FbWireOperations} implementation for this protocol version. * * @param connection - * WireConnection instance + * WireConnection instance (non-{@code null}) * @param defaultWarningMessageCallback - * Default warning message callback + * default warning message callback (non-{@code null}) * @return Wire operations implementation */ FbWireOperations createWireOperations(WireConnection connection, diff --git a/src/main/org/firebirdsql/gds/ng/wire/WireConnection.java b/src/main/org/firebirdsql/gds/ng/wire/WireConnection.java index 98e0de0d8..b9822f823 100644 --- a/src/main/org/firebirdsql/gds/ng/wire/WireConnection.java +++ b/src/main/org/firebirdsql/gds/ng/wire/WireConnection.java @@ -33,6 +33,7 @@ import org.firebirdsql.gds.ng.IAttachProperties; import org.firebirdsql.gds.ng.IConnectionProperties; import org.firebirdsql.gds.ng.LockCloseable; +import org.firebirdsql.gds.ng.WarningMessageCallback; import org.firebirdsql.gds.ng.dbcrypt.DbCryptCallback; import org.firebirdsql.gds.ng.wire.auth.ClientAuthBlock; import org.firebirdsql.gds.ng.wire.crypt.KnownServerKey; @@ -75,6 +76,7 @@ public abstract class WireConnection, C extends F // TODO Check if methods currently throwing IOException should throw SQLException instead private static final Logger log = LoggerFactory.getLogger(WireConnection.class); + private static final WarningMessageCallback NOOP_WARNING_MESSAGE_CALLBACK = warning -> {}; // Micro-optimization: we usually expect at most 3 (Firebird 5) private final List knownServerKeys = new ArrayList<>(3); @@ -487,7 +489,7 @@ void clearServerKeys() { private AbstractWireOperations getDefaultWireOperations() { ProtocolDescriptor protocolDescriptor = protocols .getProtocolDescriptor(WireProtocolConstants.PROTOCOL_VERSION10); - return (AbstractWireOperations) protocolDescriptor.createWireOperations(this, null); + return (AbstractWireOperations) protocolDescriptor.createWireOperations(this, NOOP_WARNING_MESSAGE_CALLBACK); } /** @@ -496,7 +498,7 @@ private AbstractWireOperations getDefaultWireOperations() { private FbWireOperations getCryptKeyCallbackWireOperations() { ProtocolDescriptor protocolDescriptor = protocols .getProtocolDescriptor(WireProtocolConstants.PROTOCOL_VERSION15); - return protocolDescriptor.createWireOperations(this, null); + return protocolDescriptor.createWireOperations(this, NOOP_WARNING_MESSAGE_CALLBACK); } /**