Skip to content

Commit f9be590

Browse files
jglickjglick
committed
Switching *-name.txt to a manifest attribute; and fixes related to cache maintenance
1 parent e2b6276 commit f9be590

File tree

9 files changed

+54
-75
lines changed

9 files changed

+54
-75
lines changed

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,6 @@ static URL retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriever ret
228228
listener.getLogger().println("Library " + name + "@" + version + " is due for a refresh after " + cachingMinutes + " minutes, clearing.");
229229
if (versionCacheJar.exists()) {
230230
versionCacheJar.delete();
231-
versionCacheJar.sibling(record.getDirectoryName() + "-name.txt").delete();
232231
}
233232
retrieve = true;
234233
break;
@@ -247,16 +246,13 @@ static URL retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriever ret
247246
}
248247

249248
lastReadFile.touch(System.currentTimeMillis());
250-
versionCacheJar.sibling(record.getDirectoryName() + "-name.txt").write(name, "UTF-8");
251249
versionCacheJar.copyTo(libJar);
252250
} finally {
253251
retrieveLock.readLock().unlock();
254252
}
255253
} else {
256254
retriever.retrieveJar(name, version, changelog, libJar, run, listener);
257255
}
258-
// Write the user-provided name to a file as a debugging aid.
259-
libJar.sibling(record.getDirectoryName() + "-name.txt").write(name, "UTF-8");
260256

261257
// Replace any classes requested for replay:
262258
if (!record.trusted) {

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingCleanup.java‎

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import java.io.IOException;
1010
import java.util.concurrent.TimeUnit;
1111
import java.util.concurrent.locks.ReentrantReadWriteLock;
12+
import jenkins.model.Jenkins;
1213

1314
import jenkins.util.SystemProperties;
1415

@@ -27,41 +28,28 @@ public LibraryCachingCleanup() {
2728

2829
@Override protected void execute(TaskListener listener) throws IOException, InterruptedException {
2930
FilePath globalCacheDir = LibraryCachingConfiguration.getGlobalLibrariesCacheDir();
30-
for (FilePath library : globalCacheDir.list()) {
31-
if (!removeIfExpiredCacheDirectory(library)) {
32-
// Prior to the SECURITY-2586 fix, library caches had a two-level directory structure.
33-
// These caches will never be used again, so we delete any that we find.
34-
for (FilePath version: library.list()) {
35-
if (version.child(LibraryCachingConfiguration.LAST_READ_FILE).exists()) {
36-
library.deleteRecursive();
37-
break;
38-
}
39-
}
40-
}
31+
for (FilePath libJar : globalCacheDir.list()) {
32+
removeIfExpiredCacheJar(libJar);
4133
}
34+
// Old cache directory; format has changed, so just delete it:
35+
Jenkins.get().getRootPath().child("global-libraries-cache").deleteRecursive();
4236
}
4337

4438
/**
45-
* Delete the specified cache directory if it is outdated.
46-
* @return true if specified directory is a cache directory, regardless of whether it was outdated. Used to detect
47-
* whether the cache was created before or after the fix for SECURITY-2586.
39+
* Delete the specified cache JAR if it is outdated.
4840
*/
49-
private boolean removeIfExpiredCacheDirectory(FilePath library) throws IOException, InterruptedException {
50-
final FilePath lastReadFile = new FilePath(library, LibraryCachingConfiguration.LAST_READ_FILE);
41+
private void removeIfExpiredCacheJar(FilePath libJar) throws IOException, InterruptedException {
42+
final FilePath lastReadFile = libJar.sibling(libJar.getBaseName() + "." + LibraryCachingConfiguration.LAST_READ_FILE);
5143
if (lastReadFile.exists()) {
52-
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(library.getName());
44+
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(libJar.getBaseName());
5345
retrieveLock.writeLock().lockInterruptibly();
5446
try {
5547
if (System.currentTimeMillis() - lastReadFile.lastModified() > TimeUnit.DAYS.toMillis(EXPIRE_AFTER_READ_DAYS)) {
56-
57-
library.deleteRecursive();
58-
library.withSuffix("-name.txt").delete();
48+
libJar.delete();
5949
}
6050
} finally {
6151
retrieveLock.writeLock().unlock();
6252
}
63-
return true;
6453
}
65-
return false;
6654
}
6755
}

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration.java‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,15 @@
1010
import org.kohsuke.stapler.QueryParameter;
1111

1212
import java.io.IOException;
13-
import java.io.InputStream;
14-
import java.nio.charset.StandardCharsets;
1513
import java.util.Arrays;
1614
import java.util.Collections;
1715
import java.util.List;
1816
import java.util.concurrent.TimeUnit;
1917
import java.util.concurrent.locks.ReentrantReadWriteLock;
18+
import java.util.jar.JarFile;
2019
import java.util.logging.Level;
2120
import java.util.logging.Logger;
2221

23-
import org.apache.commons.io.IOUtils;
2422
import org.apache.commons.lang.StringUtils;
2523

2624
public final class LibraryCachingConfiguration extends AbstractDescribableImpl<LibraryCachingConfiguration> {
@@ -94,26 +92,22 @@ public FormValidation doClearCache(@QueryParameter String name, @QueryParameter
9492

9593
try {
9694
if (LibraryCachingConfiguration.getGlobalLibrariesCacheDir().exists()) {
97-
for (FilePath libraryNamePath : LibraryCachingConfiguration.getGlobalLibrariesCacheDir().list("*-name.txt")) {
95+
for (FilePath libraryCachePath : LibraryCachingConfiguration.getGlobalLibrariesCacheDir().list("*.jar")) {
9896
// Libraries configured in distinct locations may have the same name. Since only admins are allowed here, this is not a huge issue, but it is probably unexpected.
9997
String cacheName;
100-
try (InputStream stream = libraryNamePath.read()) {
101-
cacheName = IOUtils.toString(stream, StandardCharsets.UTF_8);
98+
try (JarFile jf = new JarFile(libraryCachePath.getRemote())) {
99+
cacheName = jf.getManifest().getMainAttributes().getValue(LibraryRetriever.ATTR_LIBRARY_NAME);
102100
}
103101
if (cacheName.equals(name)) {
104-
FilePath libraryCachePath = LibraryCachingConfiguration.getGlobalLibrariesCacheDir()
105-
.child(libraryNamePath.getName().replace("-name.txt", ""));
106102
if (forceDelete) {
107103
LOGGER.log(Level.FINER, "Force deleting cache for {0}", name);
108-
libraryCachePath.deleteRecursive();
109-
libraryNamePath.delete();
104+
libraryCachePath.delete();
110105
} else {
111106
LOGGER.log(Level.FINER, "Safe deleting cache for {0}", name);
112107
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(libraryCachePath.getName());
113108
if (retrieveLock.writeLock().tryLock(10, TimeUnit.SECONDS)) {
114109
try {
115-
libraryCachePath.deleteRecursive();
116-
libraryNamePath.delete();
110+
libraryCachePath.delete();
117111
} finally {
118112
retrieveLock.writeLock().unlock();
119113
}

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryRetriever.java‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,20 @@
3939
import hudson.util.io.ArchiverFactory;
4040
import java.io.IOException;
4141
import java.io.OutputStream;
42+
import java.util.jar.Attributes;
43+
import java.util.jar.JarFile;
44+
import java.util.jar.Manifest;
4245

4346
/**
4447
* A way in which a library can be physically obtained for use in a build.
4548
*/
4649
public abstract class LibraryRetriever extends AbstractDescribableImpl<LibraryRetriever> implements ExtensionPoint {
4750

51+
/**
52+
* JAR manifest attribute giving original library name.
53+
*/
54+
static final String ATTR_LIBRARY_NAME = "Jenkins-Library-Name";
55+
4856
/**
4957
* Obtains library sources.
5058
* @param name the {@link LibraryConfiguration#getName}
@@ -60,7 +68,7 @@ public void retrieveJar(@NonNull String name, @NonNull String version, boolean c
6068
FilePath tmp = target.sibling(target.getBaseName() + "-checkout");
6169
try {
6270
retrieve(name, version, changelog, tmp, run, listener);
63-
dir2Jar(tmp, target);
71+
dir2Jar(name, tmp, target);
6472
} finally {
6573
tmp.deleteRecursive();
6674
WorkspaceList.tempDir(tmp).deleteRecursive();
@@ -74,28 +82,35 @@ public void retrieveJar(@NonNull String name, @NonNull String version, boolean c
7482
* Translates a historical directory with {@code src/} and/or {@code vars/} and/or {@code resources/} subdirectories
7583
* into a JAR file with Groovy in classpath orientation and {@code resources/} as a ZIP folder.
7684
*/
77-
static void dir2Jar(@NonNull FilePath dir, @NonNull FilePath jar) throws IOException, InterruptedException {
85+
static void dir2Jar(@NonNull String name, @NonNull FilePath dir, @NonNull FilePath jar) throws IOException, InterruptedException {
7886
// TODO do this more efficiently by packing JAR directly
79-
FilePath tmp2 = jar.withSuffix(".tmp2");
80-
tmp2.mkdirs();
87+
FilePath tmp = jar.sibling(jar.getBaseName() + "-repack");
88+
tmp.mkdirs();
8189
try {
8290
FilePath src = dir.child("src");
8391
if (src.isDirectory()) {
84-
src.moveAllChildrenTo(tmp2);
92+
src.moveAllChildrenTo(tmp);
8593
}
8694
FilePath vars = dir.child("vars");
8795
if (vars.isDirectory()) {
88-
vars.moveAllChildrenTo(tmp2);
96+
vars.moveAllChildrenTo(tmp);
8997
}
9098
FilePath resources = dir.child("resources");
9199
if (resources.isDirectory()) {
92-
resources.renameTo(tmp2.child("resources"));
100+
resources.renameTo(tmp.child("resources"));
101+
}
102+
try (OutputStream os = tmp.child(JarFile.MANIFEST_NAME).write()) {
103+
Manifest m = new Manifest();
104+
m.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
105+
// Informational debugging aid, since the hex JAR basename will be meaningless:
106+
m.getMainAttributes().putValue(ATTR_LIBRARY_NAME, name);
107+
m.write(os);
93108
}
94109
try (OutputStream os = jar.write()) {
95-
tmp2.archive(ArchiverFactory.ZIP, os, "**");
110+
tmp.archive(ArchiverFactory.ZIP, os, "**");
96111
}
97112
} finally {
98-
tmp2.deleteRecursive();
113+
tmp.deleteRecursive();
99114
}
100115
}
101116

‎src/main/java/org/jenkinsci/plugins/workflow/libs/SCMSourceRetriever.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ public String getLibraryPath() {
151151
if (changelog) {
152152
listener.getLogger().println("WARNING: ignoring request to compute changelog in clone mode");
153153
}
154-
doClone(scm.build(revision.getHead(), revision), libraryPath, target, run, listener);
154+
doClone(name, scm.build(revision.getHead(), revision), libraryPath, target, run, listener);
155155
} else {
156156
doRetrieve(name, changelog, scm.build(revision.getHead(), revision), libraryPath, target, run, listener);
157157
}
@@ -228,7 +228,7 @@ static void doRetrieve(String name, boolean changelog, @NonNull SCM scm, String
228228
}
229229
// Cannot add WorkspaceActionImpl to private CpsFlowExecution.flowStartNodeActions; do we care?
230230
// Copy sources with relevant files from the checkout:
231-
LibraryRetriever.dir2Jar(lease.path.child(libraryPath), target);
231+
LibraryRetriever.dir2Jar(name, lease.path.child(libraryPath), target);
232232
}
233233
}
234234

@@ -240,7 +240,7 @@ private static String getFilePathSuffix() {
240240
/**
241241
* Similar to {@link #doRetrieve} but used in {@link #clone} mode.
242242
*/
243-
private static void doClone(@NonNull SCM scm, String libraryPath, FilePath target, Run<?, ?> run, TaskListener listener) throws Exception {
243+
private static void doClone(@NonNull String name, @NonNull SCM scm, String libraryPath, FilePath target, Run<?, ?> run, TaskListener listener) throws Exception {
244244
// TODO merge into doRetrieve
245245
SCMStep delegate = new GenericSCMStep(scm);
246246
delegate.setPoll(false);
@@ -261,7 +261,7 @@ private static void doClone(@NonNull SCM scm, String libraryPath, FilePath targe
261261
root = tmp.child(libraryPath);
262262
}
263263
// TODO handle INCLUDE_SRC_TEST_IN_LIBRARIES
264-
LibraryRetriever.dir2Jar(root, target);
264+
LibraryRetriever.dir2Jar(name, root, target);
265265
} finally {
266266
tmp.deleteRecursive();
267267
WorkspaceList.tempDir(tmp).deleteRecursive();

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingCleanupTest.java‎

Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939

4040
import static org.hamcrest.MatcherAssert.assertThat;
4141
import static org.hamcrest.Matchers.not;
42-
import static org.hamcrest.io.FileMatchers.anExistingDirectory;
4342
import static org.hamcrest.io.FileMatchers.anExistingFile;
4443

4544
public class LibraryCachingCleanupTest {
@@ -67,28 +66,16 @@ public void smokes() throws Throwable {
6766
WorkflowRun b = r.buildAndAssertSuccess(p);
6867
LibrariesAction action = b.getAction(LibrariesAction.class);
6968
LibraryRecord record = action.getLibraries().get(0);
70-
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName());
71-
assertThat(new File(cache.getRemote()), anExistingDirectory());
69+
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName() + ".jar");
70+
assertThat(new File(cache.getRemote()), anExistingFile());
7271
// Run LibraryCachingCleanup and show that cache is not deleted.
7372
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
74-
assertThat(new File(cache.getRemote()), anExistingDirectory());
75-
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), anExistingFile());
73+
assertThat(new File(cache.getRemote()), anExistingFile());
7674
// Run LibraryCachingCleanup after modifying LAST_READ_FILE to be an old date and and show that cache is deleted.
7775
long oldMillis = ZonedDateTime.now().minusDays(LibraryCachingCleanup.EXPIRE_AFTER_READ_DAYS + 1).toInstant().toEpochMilli();
78-
cache.child(LibraryCachingConfiguration.LAST_READ_FILE).touch(oldMillis);
76+
cache.sibling(cache.getBaseName() + "." + LibraryCachingConfiguration.LAST_READ_FILE).touch(oldMillis);
7977
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
80-
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
81-
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), not(anExistingDirectory()));
82-
}
83-
84-
@Test
85-
public void preSecurity2586() throws Throwable {
86-
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child("name").child("version");
87-
cache.mkdirs();
88-
cache.child(LibraryCachingConfiguration.LAST_READ_FILE).touch(System.currentTimeMillis());
89-
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
90-
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
91-
assertThat(new File(cache.getParent().getRemote()), not(anExistingDirectory()));
78+
assertThat(new File(cache.getRemote()), not(anExistingFile()));
9279
}
9380

9481
}

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfigurationTest.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,9 @@ public void clearCache() throws Exception {
175175
LibraryRecord record = action.getLibraries().get(0);
176176
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName());
177177
assertThat(new File(cache.getRemote()), anExistingDirectory());
178-
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), anExistingFile());
179178
// Clear the cache. TODO: Would be more realistic to set up security and use WebClient.
180179
ExtensionList.lookupSingleton(LibraryCachingConfiguration.DescriptorImpl.class).doClearCache("library", false);
181180
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
182-
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), not(anExistingFile()));
183181
}
184182

185183
}

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryRetrieverTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,13 @@ private void assertDir2Jar(Set<String> inputs, Set<String> outputs) throws Excep
6060
dir.child(input).write("xxx", null);
6161
}
6262
FilePath jar = work.child("x.jar");
63-
LibraryRetriever.dir2Jar(dir, jar);
63+
LibraryRetriever.dir2Jar("mylib", dir, jar);
6464
Set<String> actualOutputs = new TreeSet<>();
6565
try (JarFile jf = new JarFile(jar.getRemote())) {
66+
assertThat(jf.getManifest().getMainAttributes().getValue(LibraryRetriever.ATTR_LIBRARY_NAME), is("mylib"));
6667
jf.stream().forEach(e -> {
6768
String name = e.getName();
68-
if (!name.endsWith("/")) {
69+
if (!name.endsWith("/") && !name.startsWith("META-INF/")) {
6970
actualOutputs.add(name);
7071
}
7172
});

‎src/test/java/org/jenkinsci/plugins/workflow/libs/SCMSourceRetrieverTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@
7979
import org.jvnet.hudson.test.WithoutJenkins;
8080

8181
import static org.hamcrest.MatcherAssert.assertThat;
82-
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
82+
import static org.hamcrest.Matchers.arrayContaining;
8383
import static org.hamcrest.Matchers.equalTo;
8484
import static org.hamcrest.Matchers.matchesPattern;
8585
import static org.hamcrest.Matchers.nullValue;
@@ -392,7 +392,7 @@ public static class BasicSCMSource extends SCMSource {
392392
r.assertLogContains("something special", b);
393393
r.assertLogContains("Using shallow clone with depth 1", b);
394394
// Fails to reproduce presence of *.jar.tmp@tmp; probably specific to use of GIT_ASKPASS:
395-
assertThat(new File(b.getRootDir(), "libs").list(), arrayContainingInAnyOrder(matchesPattern("[0-9a-f]{64}[.]jar"), matchesPattern("[0-9a-f]{64}-name[.]txt")));
395+
assertThat(new File(b.getRootDir(), "libs").list(), arrayContaining(matchesPattern("[0-9a-f]{64}[.]jar")));
396396
}
397397

398398
@Test public void cloneModeLibraryPath() throws Exception {

0 commit comments

Comments
 (0)