From 2580bc2aa47bf03416a5f13868c665e4d1b771b6 Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Tue, 23 Apr 2024 16:49:49 -0700 Subject: [PATCH 1/3] vault: add exception handling inside child iterator to protect against leaked connections if listing fails --- vault/VERSION | 2 +- vault/build.gradle | 2 +- .../opencadc/vault/NodePersistenceImpl.java | 28 +++++++++++++------ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/vault/VERSION b/vault/VERSION index c5fd5316..5ff92dd7 100644 --- a/vault/VERSION +++ b/vault/VERSION @@ -4,6 +4,6 @@ # tags with and without build number so operators use the versioned # tag but we always keep a timestamped tag in case a semantic tag gets # replaced accidentally -VER=1.0.3 +VER=1.0.4 TAGS="${VER} ${VER}-$(date --utc +"%Y%m%dT%H%M%S")" unset VER diff --git a/vault/build.gradle b/vault/build.gradle index f9a97d23..fd1513c4 100644 --- a/vault/build.gradle +++ b/vault/build.gradle @@ -35,7 +35,7 @@ dependencies { compile 'org.opencadc:cadc-log:[1.1.6,2.0)' compile 'org.opencadc:cadc-gms:[1.0.5,)' compile 'org.opencadc:cadc-rest:[1.3.16,)' - compile 'org.opencadc:cadc-vos:[2.0.6,)' + compile 'org.opencadc:cadc-vos:[2.0.7,)' compile 'org.opencadc:cadc-vos-server:[2.0.9,)' compile 'org.opencadc:cadc-vosi:[1.3.2,)' compile 'org.opencadc:cadc-uws:[1.0,)' diff --git a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java index 91ed9b69..3a9b151c 100644 --- a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java +++ b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java @@ -476,6 +476,7 @@ private class ChildNodeWrapper implements ResourceIterator { private final ContainerNode parent; private final ResourceIterator childIter; + private boolean closedForException = false; private final IdentityManager identityManager = AuthenticationUtil.getIdentityManager(); private final Map identCache = new TreeMap<>(); @@ -497,7 +498,7 @@ private class ChildNodeWrapper implements ResourceIterator { @Override public boolean hasNext() { - return childIter.hasNext(); + return !closedForException && childIter.hasNext(); } @Override @@ -505,14 +506,25 @@ public Node next() { Node ret = childIter.next(); ret.parent = parent; - // owner - Subject s = identCache.get(ret.ownerID); - if (s == null) { - s = identityManager.toSubject(ret.ownerID); - identCache.put(ret.ownerID, s); + try { + // owner + Subject s = identCache.get(ret.ownerID); + if (s == null) { + s = identityManager.toSubject(ret.ownerID); + identCache.put(ret.ownerID, s); + } + ret.owner = s; + ret.ownerDisplay = identityManager.toDisplayString(ret.owner); + } catch (RuntimeException ex) { + try { + // abort + close(); + } catch (Exception cex) { + log.error("failed to close iterator", ex); + } + closedForException = true; + throw ex; } - ret.owner = s; - ret.ownerDisplay = identityManager.toDisplayString(ret.owner); if (ret instanceof DataNode) { DataNode dn = (DataNode) ret; From 71fc17ddb9cbae5d2becd934a7b7650da1cd08af Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Tue, 24 Sep 2024 14:16:38 -0700 Subject: [PATCH 2/3] vault: handle exception in childIterator fix NPE on first run of DataNodeSizeWorkerSync fix no content check in files GetAction --- .../java/org/opencadc/vault/NodePersistenceImpl.java | 2 +- .../main/java/org/opencadc/vault/files/GetAction.java | 10 +++++++++- .../org/opencadc/vault/metadata/BackgroundLogInfo.java | 10 ++++++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java index ace723d0..54dd0348 100644 --- a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java +++ b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java @@ -498,7 +498,7 @@ public ResourceIterator iterator(ContainerNode parent, Integer limit, Stri private class ChildNodeWrapper implements ResourceIterator { private final ContainerNode parent; - private final ResourceIterator childIter; + private ResourceIterator childIter; private boolean closedForException = false; private final IdentityManager identityManager = AuthenticationUtil.getIdentityManager(); diff --git a/vault/src/main/java/org/opencadc/vault/files/GetAction.java b/vault/src/main/java/org/opencadc/vault/files/GetAction.java index 7d740d7d..f4dd2895 100644 --- a/vault/src/main/java/org/opencadc/vault/files/GetAction.java +++ b/vault/src/main/java/org/opencadc/vault/files/GetAction.java @@ -111,7 +111,7 @@ public void doAction() throws Exception { boolean noArtifact = node.bytesUsed == null || node.bytesUsed == 0L; noArtifact = noArtifact && nodePersistence.preventNotFound && rn.artifact == null; if (noArtifact) { - // no file + // no artifact found by preventNotFound syncOutput.setCode(HttpURLConnection.HTTP_NO_CONTENT); return; } @@ -126,6 +126,14 @@ public void doAction() throws Exception { rn.protos = tg.getEndpoints(targetURI, pullTransfer, null); rn.artifact = tg.resolvedArtifact; // currently unused at this point } + // check artifact again (!preventNotFound) + noArtifact = node.bytesUsed == null || node.bytesUsed == 0L; + noArtifact = noArtifact && rn.artifact == null; + if (noArtifact) { + // no file + syncOutput.setCode(HttpURLConnection.HTTP_NO_CONTENT); + return; + } if (rn.protos.isEmpty()) { throw new TransientException("No location found for file " + Utils.getPath(node)); diff --git a/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java b/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java index 88ac11a9..92e0752e 100644 --- a/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java +++ b/vault/src/main/java/org/opencadc/vault/metadata/BackgroundLogInfo.java @@ -98,9 +98,11 @@ public void setOperation(String op) { } public void setLastModified(Date ts) { - DateFormat df = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC); - this.lastmodified = df.format(ts); + if (ts != null) { + DateFormat df = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC); + this.lastmodified = df.format(ts); + } else { + this.lastmodified = null; + } } - - } From 32be4e4b90294ec1a5e4f36855fd748b4bc81797 Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Tue, 24 Sep 2024 14:41:06 -0700 Subject: [PATCH 3/3] extra NPE prevention check --- .../main/java/org/opencadc/vault/NodePersistenceImpl.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java index 54dd0348..3b0f4821 100644 --- a/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java +++ b/vault/src/main/java/org/opencadc/vault/NodePersistenceImpl.java @@ -521,11 +521,7 @@ private class ChildNodeWrapper implements ResourceIterator { @Override public boolean hasNext() { - return !closedForException && childIter.hasNext(); - //if (childIter != null) { - // return childIter.hasNext(); - //} - //return false; + return !closedForException && childIter != null && childIter.hasNext(); } @Override