Skip to content

Commit

Permalink
Fix some SonarLint findings
Browse files Browse the repository at this point in the history
Simple ones:

- Logging statements: use placeholders, not string concatenation
- Abstract classes: use protected, not public, constructors
- Some simplified lambdas
- Some simplified exception catching
- Use StringBuilder instead of StringBuffer
- Use BigInteger.valueOf() instead of new BigInteger()
- Use "byte[] buf" instead of "byte buf[]"

There are still a lot of more findings some of them worrying, such as
throwing exceptions in finally blocks, or catching Error or Throwable
in several places, or others.
  • Loading branch information
tomaswolf committed Oct 30, 2022
1 parent 4037290 commit b19a3ed
Show file tree
Hide file tree
Showing 101 changed files with 334 additions and 526 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -695,10 +695,10 @@ protected synchronized void setOutputStream(OutputStream out) throws SecurityExc
fh.setLevel(Level.FINEST);
fh.setFormatter(new Formatter() {
@Override
public String format(LogRecord record) {
String message = formatMessage(record);
public String format(LogRecord logRecord) {
String message = formatMessage(logRecord);
String throwable = "";
Throwable t = record.getThrown();
Throwable t = logRecord.getThrown();
if (t != null) {
StringWriter sw = new StringWriter();
try (PrintWriter pw = new PrintWriter(sw)) {
Expand All @@ -708,8 +708,8 @@ public String format(LogRecord record) {
throwable = sw.toString();
}
return String.format("%1$tY-%1$tm-%1$td: %2$-7.7s: %3$-32.32s: %4$s%5$s%n",
new Date(record.getMillis()), record.getLevel().getName(),
record.getLoggerName(), message, throwable);
new Date(logRecord.getMillis()), logRecord.getLevel().getName(),
logRecord.getLoggerName(), message, throwable);
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,7 @@ public static List<SubsystemFactory> resolveServerSubsystems(
continue;
}

factory = registerSubsystemFactoryListeners(
server, level, stdout, stderr, options, factory);
registerSubsystemFactoryListeners(server, level, stdout, stderr, options, factory);
subsystems.add(factory);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
public abstract class ServerEventListenerHelper extends AbstractLoggingBean implements NamedResource {
private final String name;

public ServerEventListenerHelper(String name, Logger logger) {
protected ServerEventListenerHelper(String name, Logger logger) {
super(logger);

this.name = name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ public void establishedDynamicTunnel(
throws IOException {
if (reason == null) {
if (log.isInfoEnabled()) {
log.info("Estalibshed dynamic tunnel for session={}: local={}, bound={}", session, local, boundAddress);
log.info("Estalibshed dynamic tunnel for session={}: local={} bound={}", session, local, boundAddress);
}
} else {
log.error("Failed ({}) to establish dynamic tunnel for session={}, bound={}: {}",
log.error("Failed ({}) to establish dynamic tunnel for session={}: local={} bound={}: {}",
reason.getClass().getSimpleName(), session, local, boundAddress, reason.getMessage());
}
}
Expand All @@ -86,7 +86,7 @@ public void tornDownDynamicTunnel(
throws IOException {
if (reason == null) {
if (log.isInfoEnabled()) {
log.info("Torn down dynamic tunnel for session={}: address={}", session);
log.info("Torn down dynamic tunnel for session={}: address={}", session, address);
}
} else {
log.error("Failed ({}) to tear down dynamic tunnel for session={}, address={}: {}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ static HostKeyIdentityProvider wrap(KeyPair... pairs) {
return wrap(GenericUtils.asList(pairs));
}

static HostKeyIdentityProvider wrap(Iterable<? extends KeyPair> pairs) {
static HostKeyIdentityProvider wrap(Iterable<KeyPair> pairs) {
return session -> GenericUtils.wrapIterable(pairs,
kp -> new SimpleImmutableEntry<>(kp, Collections.<X509Certificate> emptyList()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,9 @@ public boolean isHostMatch(String host, int port) {
byte[] expected = getDigestValue();
byte[] actual = calculateHashValue(host, port, getDigester(), getSaltValue());
return Arrays.equals(expected, actual);
} catch (RuntimeException e) {
throw e;
} catch (Throwable t) {
if (t instanceof RuntimeException) {
throw (RuntimeException) t;
}
throw new RuntimeSshException(
"Failed (" + t.getClass().getSimpleName() + ")" + " to calculate hash value: " + t.getMessage(), t);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public Iterable<KeyPair> loadKeys(SessionContext session) {
return loadKeys(session, null);
}

protected Iterable<KeyPair> loadKeys(SessionContext session, Predicate<? super KeyPair> filter) {
protected Iterable<KeyPair> loadKeys(SessionContext session, Predicate<KeyPair> filter) {
return ClientIdentityProvider.lazyKeysLoader(providers, p -> doGetKeyPairs(session, p), filter);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ public Iterable<KeyPair> getClientIdentities(SessionContext session)
return identitiesHolder.get();
}

Iterable<KeyPair> kp = identitiesHolder.getAndSet(null); // start fresh
identitiesHolder.set(null); // start fresh
Path path = getPath();
if (!exists()) {
return identitiesHolder.get();
}

kp = reloadClientIdentities(session, path);
Iterable<KeyPair> kp = reloadClientIdentities(session, path);
updateReloadAttributes();
identitiesHolder.set(kp);
return kp;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ static ClientIdentityProvider of(KeyPair kp) {
*/
static Iterable<KeyPair> lazyKeysLoader(
Iterable<? extends ClientIdentityProvider> providers,
Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> kpExtractor,
Predicate<? super KeyPair> filter) {
Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> kpExtractor, Predicate<KeyPair> filter) {
Objects.requireNonNull(kpExtractor, "No key pair extractor provided");
if (providers == null) {
return Collections.emptyList();
Expand Down Expand Up @@ -114,8 +113,7 @@ public String toString() {
*/
static Iterator<KeyPair> lazyKeysIterator(
Iterator<? extends ClientIdentityProvider> providers,
Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> kpExtractor,
Predicate<? super KeyPair> filter) {
Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> kpExtractor, Predicate<KeyPair> filter) {
Objects.requireNonNull(kpExtractor, "No key pair extractor provided");
return (providers == null)
? Collections.emptyIterator()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@
*/
public class LazyClientIdentityIterator implements Iterator<KeyPair> {
protected boolean finished;
protected Iterator<? extends KeyPair> currentIdentities;
protected Iterator<KeyPair> currentIdentities;
protected KeyPair currentPair;

private final Iterator<? extends ClientIdentityProvider> providers;
private final Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> kpExtractor;
private final Predicate<? super KeyPair> filter;
private final Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> kpExtractor;
private final Predicate<KeyPair> filter;

/**
* @param providers The providers - ignored if {@code null}
Expand All @@ -52,10 +52,9 @@ public class LazyClientIdentityIterator implements Iterator<KeyPair> {
* @param filter Any further filter to apply on (non-{@code null}) key pairs before returning it as the
* {@link Iterator#next()} result.
*/
public LazyClientIdentityIterator(
Iterator<? extends ClientIdentityProvider> providers,
Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> kpExtractor,
Predicate<? super KeyPair> filter) {
public LazyClientIdentityIterator(Iterator<? extends ClientIdentityProvider> providers,
Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> kpExtractor,
Predicate<KeyPair> filter) {
this.providers = providers;
this.kpExtractor = Objects.requireNonNull(kpExtractor, "No key pair extractor provided");
this.filter = filter;
Expand All @@ -65,11 +64,11 @@ public Iterator<? extends ClientIdentityProvider> getProviders() {
return providers;
}

public Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> getIdentitiesExtractor() {
public Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> getIdentitiesExtractor() {
return kpExtractor;
}

public Predicate<? super KeyPair> getFilter() {
public Predicate<KeyPair> getFilter() {
return filter;
}

Expand All @@ -90,15 +89,15 @@ public boolean hasNext() {
return true;
}

Function<? super ClientIdentityProvider, ? extends Iterable<? extends KeyPair>> x = getIdentitiesExtractor();
Predicate<? super KeyPair> f = getFilter();
Function<? super ClientIdentityProvider, ? extends Iterable<KeyPair>> x = getIdentitiesExtractor();
Predicate<KeyPair> f = getFilter();
while (provs.hasNext()) {
ClientIdentityProvider p = provs.next();
if (p == null) {
continue;
}

Iterable<? extends KeyPair> ids = x.apply(p);
Iterable<KeyPair> ids = x.apply(p);
currentIdentities = (ids == null) ? null : ids.iterator();
currentPair = KeyIdentityProvider.exhaustCurrentIdentities(currentIdentities);
if (currentPair == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,20 +301,17 @@ public enum PtyMode {
* A {@code null}-safe {@link ToIntFunction} that returns the {@link PtyMode#toInt()} value and (-1) for
* {@code null}
*/
public static final ToIntFunction<PtyMode> OPCODE_EXTRACTOR = v -> (v == null) ? -1 : v.toInt();
public static final ToIntFunction<PtyMode> OPCODE_EXTRACTOR = op -> (op == null) ? -1 : op.toInt();

/**
* A {@code null}-safe {@link Comparator} of {@link PtyMode} values according to their {@link PtyMode#toInt()} value
*
* @see #OPCODE_EXTRACTOR
*/
public static final Comparator<PtyMode> BY_OPCODE = new Comparator<PtyMode>() {
@Override
public int compare(PtyMode o1, PtyMode o2) {
int v1 = OPCODE_EXTRACTOR.applyAsInt(o1);
int v2 = OPCODE_EXTRACTOR.applyAsInt(o2);
return Integer.compare(v1, v2);
}
public static final Comparator<PtyMode> BY_OPCODE = (o1, o2) -> {
int v1 = OPCODE_EXTRACTOR.applyAsInt(o1);
int v2 = OPCODE_EXTRACTOR.applyAsInt(o2);
return Integer.compare(v1, v2);
};

private final int v;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private static NavigableMap<String, String> loadVersionProperties(
log.warn("Failed ({}) to load version properties from {}: {}",
e.getClass().getSimpleName(), cl, e.getMessage());
if (log.isDebugEnabled()) {
log.debug("Version property failure details for loader=" + cl, e);
log.debug("Version property failure details for loader={}", cl, e);
}
continue;
}
Expand All @@ -85,7 +85,7 @@ private static NavigableMap<String, String> loadVersionProperties(
String prev = result.put(key, value);
if (prev != null) {
Logger log = LoggerFactory.getLogger(anchor);
log.warn("Multiple values for key=" + key + ": current=" + value + ", previous=" + prev);
log.warn("Multiple values for key={}: current={}, previous={}", key, value, prev);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public final class KeyUtils {
/**
* The most commonly used RSA public key exponent
*/
public static final BigInteger DEFAULT_RSA_PUBLIC_EXPONENT = new BigInteger("65537");
public static final BigInteger DEFAULT_RSA_PUBLIC_EXPONENT = BigInteger.valueOf(65537);

/**
* Name of algorithm for DSS keys to be used when calling security provider
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ protected final boolean parseBooleanHeader(Map<String, String> headers, String p
try {
boolVal = PropertyResolverUtils.parseBoolean(stringVal);
} catch (IllegalArgumentException e) {
log.warn("Ignoring non-boolean property value for \"" + propertyKey + "\": " + stringVal);
log.warn("Ignoring non-boolean property value for \"{}\": {}", propertyKey, stringVal);
boolVal = null;
}
if (boolVal == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,7 @@ public DSAPrivateKey decodePrivateKey(
Objects.requireNonNull(y, "No public key data"); // TODO run some validation on it
BigInteger x = KeyEntryResolver.decodeBigInt(keyData);

try {
return generatePrivateKey(new DSAPrivateKeySpec(x, p, q, g));
} finally {
// get rid of sensitive data a.s.a.p
p = null;
q = null;
g = null;
y = null;
x = null;
}
return generatePrivateKey(new DSAPrivateKeySpec(x, p, q, g));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,7 @@ public ECPrivateKey decodePrivateKey(
Objects.requireNonNull(pubKey, "No public point"); // TODO validate it is a valid ECPoint
BigInteger s = KeyEntryResolver.decodeBigInt(keyData);
ECParameterSpec params = curve.getParameters();
try {
return generatePrivateKey(new ECPrivateKeySpec(s, params));
} finally {
// get rid of sensitive data a.s.a.p
s = null;
}
return generatePrivateKey(new ECPrivateKeySpec(s, params));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public class OpenSSHRSAPrivateKeyDecoder extends AbstractPrivateKeyEntryDecoder<RSAPublicKey, RSAPrivateKey> {
public static final BigInteger DEFAULT_PUBLIC_EXPONENT = new BigInteger("65537");
public static final BigInteger DEFAULT_PUBLIC_EXPONENT = KeyUtils.DEFAULT_RSA_PUBLIC_EXPONENT;
public static final OpenSSHRSAPrivateKeyDecoder INSTANCE = new OpenSSHRSAPrivateKeyDecoder();

public OpenSSHRSAPrivateKeyDecoder() {
Expand Down Expand Up @@ -79,16 +79,8 @@ public RSAPrivateKey decodePrivateKey(
if (!Objects.equals(n, modulus)) {
log.warn("decodePrivateKey({}) mismatched modulus values: encoded={}, calculated={}", keyType, n, modulus);
}
try {
return generatePrivateKey(new RSAPrivateCrtKeySpec(
n, e, d, p, q, d.mod(p.subtract(BigInteger.ONE)), d.mod(q.subtract(BigInteger.ONE)), inverseQmodP));
} finally {
// get rid of sensitive data a.s.a.p
d = null;
inverseQmodP = null;
p = null;
q = null;
}
return generatePrivateKey(new RSAPrivateCrtKeySpec(n, e, d, p, q, d.mod(p.subtract(BigInteger.ONE)),
d.mod(q.subtract(BigInteger.ONE)), inverseQmodP));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public abstract class BaseFileSystem<T extends Path> extends FileSystem {
protected final Logger log;
private final FileSystemProvider fileSystemProvider;

public BaseFileSystem(FileSystemProvider fileSystemProvider) {
protected BaseFileSystem(FileSystemProvider fileSystemProvider) {
this.log = LoggerFactory.getLogger(getClass());
this.fileSystemProvider = Objects.requireNonNull(fileSystemProvider, "No file system provider");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public abstract class BasePath<T extends BasePath<T, FS>, FS extends BaseFileSys
private String strValue;
private int hashValue;

public BasePath(FS fileSystem, String root, List<String> names) {
protected BasePath(FS fileSystem, String root, List<String> names) {
this.fileSystem = Objects.requireNonNull(fileSystem, "No file system provided");
this.root = root;
this.names = names;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.apache.sshd.common.io;

import java.io.IOException;
import java.util.Objects;

import org.apache.sshd.common.SshException;
import org.apache.sshd.common.future.DefaultVerifiableSshFuture;
Expand Down Expand Up @@ -62,4 +63,11 @@ public Throwable getException() {
return null;
}
}

public static IoWriteFuture fulfilled(Object id, Object value) {
AbstractIoWriteFuture result = new AbstractIoWriteFuture(id, null) {
};
result.setValue(Objects.requireNonNull(value));
return result;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ protected void resetCacheMap(Collection<?> resources) {
}

if (log.isDebugEnabled()) {
log.debug("resetCacheMap(" + resources + ") removed previous cached keys for " + toDelete);
log.debug("resetCacheMap({}) removed previous cached keys for {}", resources, toDelete);
}
}

Expand Down Expand Up @@ -208,10 +208,8 @@ public boolean hasNext() {

@Override
public KeyPair next() {
if (!nextKeyPairSet) {
if (!setNextObject()) {
throw new NoSuchElementException("Out of files to try");
}
if (!nextKeyPairSet && !setNextObject()) {
throw new NoSuchElementException("Out of files to try");
}
nextKeyPairSet = false;
return nextKeyPair;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ static KeyIdentityProvider wrapKeyPairs(Iterable<KeyPair> pairs) {
* @return The first non-{@code null} key pair found in the iterator - {@code null} if all elements exhausted
* without such an entry
*/
static KeyPair exhaustCurrentIdentities(Iterator<? extends KeyPair> ids) {
static KeyPair exhaustCurrentIdentities(Iterator<KeyPair> ids) {
while ((ids != null) && ids.hasNext()) {
KeyPair kp = ids.next();
if (kp != null) {
Expand Down
Loading

0 comments on commit b19a3ed

Please sign in to comment.