From 284e69efd020d853a7669c4694df15ba56a68085 Mon Sep 17 00:00:00 2001 From: Adolfo Santos Date: Wed, 30 Mar 2022 13:19:44 -0700 Subject: [PATCH] fix BUCK_CLASSPATH limit on linux (#2692) Summary: Pull Request resolved: https://github.com/facebook/buck/pull/2692 When running `buck test ...` the RemoteExecution tests fails with **"Argument list too long"**. Reason: The concatanation of paths used as BUCK_CLASSPATH is crossing the limit emposed by linux for the env variable values. (https://github.com/torvalds/linux/blob/master/include/uapi/linux/limits.h#L8) How to reproduce the problem on a devserver: - execute jshell ``` /usr/local/java-runtime/impl/11/bin/jshell ``` - paste the script that spawn a process with an env value ``` int test(String env, int size) throws Exception { var p = new ProcessBuilder("ls"); p.environment().put(env, "b".repeat(size)); return p.start().waitFor(); } test("BUCK_CLASSPATH", 131_056) // ok: 0 test("BUCK_CLASSPATH", 131_057) // fail: 7 - Argument list too long ``` Reviewed By: bobyangyf fbshipit-source-id: 82be3046d32894b9449bfec84c7cf1cc1e87f798 --- .../bootstrapper/ClassLoaderBootstrapper.java | 28 +----- .../cli/bootstrapper/ClassLoaderFactory.java | 92 +++++++++++++++++++ .../ModernBuildRuleRemoteExecutionHelper.java | 10 +- .../buck/rules/modern/builders/trampoline.sh | 3 + .../facebook/buck/util/env/BuckClasspath.java | 37 ++++++++ .../bootstrapper/ClassLoaderFactoryTest.java | 76 +++++++++++++++ .../buck/util/env/BuckClasspathTest.java | 37 ++++++++ .../environment/EnvironmentFilter.java | 1 + 8 files changed, 249 insertions(+), 35 deletions(-) create mode 100644 src/com/facebook/buck/cli/bootstrapper/ClassLoaderFactory.java create mode 100644 test/com/facebook/buck/cli/bootstrapper/ClassLoaderFactoryTest.java diff --git a/src/com/facebook/buck/cli/bootstrapper/ClassLoaderBootstrapper.java b/src/com/facebook/buck/cli/bootstrapper/ClassLoaderBootstrapper.java index ff18205b1c4..85a135a5440 100644 --- a/src/com/facebook/buck/cli/bootstrapper/ClassLoaderBootstrapper.java +++ b/src/com/facebook/buck/cli/bootstrapper/ClassLoaderBootstrapper.java @@ -16,11 +16,6 @@ package com.facebook.buck.cli.bootstrapper; -import java.io.File; -import java.net.MalformedURLException; -import java.net.URL; -import java.net.URLClassLoader; -import java.nio.file.Paths; import java.util.Arrays; /** @@ -39,7 +34,7 @@ */ public final class ClassLoaderBootstrapper { - private static final ClassLoader classLoader = createClassLoader(); + private static final ClassLoader classLoader = ClassLoaderFactory.withEnv().create(); private ClassLoaderBootstrapper() {} @@ -60,25 +55,4 @@ public static Class loadClass(String name) { throw new NoClassDefFoundError(name); } } - - @SuppressWarnings("PMD.BlacklistedSystemGetenv") - private static ClassLoader createClassLoader() { - // BUCK_CLASSPATH is not set by a user, no need to use EnvVariablesProvider. - String classPath = System.getenv("BUCK_CLASSPATH"); - if (classPath == null) { - throw new RuntimeException("BUCK_CLASSPATH not set"); - } - - String[] strings = classPath.split(File.pathSeparator); - URL[] urls = new URL[strings.length]; - for (int i = 0; i < urls.length; i++) { - try { - urls[i] = Paths.get(strings[i]).toUri().toURL(); - } catch (MalformedURLException e) { - throw new RuntimeException(e); - } - } - - return new URLClassLoader(urls); - } } diff --git a/src/com/facebook/buck/cli/bootstrapper/ClassLoaderFactory.java b/src/com/facebook/buck/cli/bootstrapper/ClassLoaderFactory.java new file mode 100644 index 00000000000..e9a4b40b086 --- /dev/null +++ b/src/com/facebook/buck/cli/bootstrapper/ClassLoaderFactory.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.buck.cli.bootstrapper; + +import static java.util.Objects.requireNonNull; + +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.file.Paths; +import java.util.function.Function; +import java.util.stream.Stream; + +/** Creates a ClassLoader from {@code System.getenv()}. classpath entries. */ +public class ClassLoaderFactory { + + /** Expose a provider to facilitate mock tests. */ + @FunctionalInterface + public interface ClassPathProvider extends Function {} + + static final String BUCK_CLASSPATH = "BUCK_CLASSPATH"; + static final String EXTRA_BUCK_CLASSPATH = "EXTRA_BUCK_CLASSPATH"; + + private final ClassPathProvider classPathProvider; + + @SuppressWarnings("PMD.BlacklistedSystemGetenv") + static ClassLoaderFactory withEnv() { + return new ClassLoaderFactory(System.getenv()::get); + } + + ClassLoaderFactory(ClassPathProvider classPathProvider) { + this.classPathProvider = requireNonNull(classPathProvider); + } + + /** + * Create a new ClassLoader that concats {@value BUCK_CLASSPATH} and {@value EXTRA_BUCK_CLASSPATH} + * + * @return ClassLoader instance to env classpath. + */ + public ClassLoader create() { + // BUCK_CLASSPATH is not set by a user, no need to use EnvVariablesProvider. + String classPath = classPathProvider.apply(BUCK_CLASSPATH); + String extraClassPath = classPathProvider.apply(EXTRA_BUCK_CLASSPATH); + + if (classPath == null) { + throw new RuntimeException(BUCK_CLASSPATH + " not set"); + } + + URL[] urls = + Stream.of(classPath, extraClassPath) + .flatMap(this::splitPaths) + .filter(this::nonBlank) + .map(this::toUrl) + .toArray(URL[]::new); + + return new URLClassLoader(urls); + } + + private Stream splitPaths(String paths) { + if (paths == null) { + return Stream.empty(); + } + return Stream.of(paths.split(File.pathSeparator)); + } + + private boolean nonBlank(String path) { + return !path.isBlank(); + } + + private URL toUrl(String path) { + try { + return Paths.get(path).toUri().toURL(); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + } +} diff --git a/src/com/facebook/buck/rules/modern/builders/ModernBuildRuleRemoteExecutionHelper.java b/src/com/facebook/buck/rules/modern/builders/ModernBuildRuleRemoteExecutionHelper.java index 9063f28306d..560e91edf50 100644 --- a/src/com/facebook/buck/rules/modern/builders/ModernBuildRuleRemoteExecutionHelper.java +++ b/src/com/facebook/buck/rules/modern/builders/ModernBuildRuleRemoteExecutionHelper.java @@ -62,7 +62,6 @@ import com.facebook.buck.util.function.ThrowingSupplier; import com.facebook.buck.util.hashing.FileHashLoader; import com.facebook.buck.util.types.Pair; -import com.google.common.base.Joiner; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -74,7 +73,6 @@ import com.google.common.hash.HashFunction; import com.google.common.io.ByteStreams; import java.io.ByteArrayInputStream; -import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; @@ -694,8 +692,8 @@ private ImmutableSortedMap getBuilderEnvironmentOverrides( String relativePluginRoot = relativizePathString(cellPrefixRoot, PLUGIN_ROOT); String relativePluginResources = relativizePathString(cellPrefixRoot, PLUGIN_RESOURCES); return ImmutableSortedMap.naturalOrder() - .put("CLASSPATH", classpathArg(bootstrapClasspath)) - .put("BUCK_CLASSPATH", classpathArg(classpath)) + .putAll(BuckClasspath.getClasspathEnv(classpath)) + .putAll(BuckClasspath.getBootstrapClasspathEnv(bootstrapClasspath)) .put( "EXTRA_JAVA_ARGS", ManagementFactory.getRuntimeMXBean().getInputArguments().stream() @@ -870,8 +868,4 @@ private ThrowingSupplier prepareClassPath( }, IOException.class); } - - private static String classpathArg(Iterable classpath) { - return Joiner.on(File.pathSeparator).join(classpath); - } } diff --git a/src/com/facebook/buck/rules/modern/builders/trampoline.sh b/src/com/facebook/buck/rules/modern/builders/trampoline.sh index 30a4d29f424..4baa62c630d 100755 --- a/src/com/facebook/buck/rules/modern/builders/trampoline.sh +++ b/src/com/facebook/buck/rules/modern/builders/trampoline.sh @@ -33,9 +33,12 @@ function getJavaPathForVersion() { } export BUCK_CLASSPATH=$(resolveList $BUCK_CLASSPATH) +BUCK_CLASSPATH_EXTRA=$(resolveList "$BUCK_CLASSPATH_EXTRA") export CLASSPATH=$(resolveList $CLASSPATH) export BUCK_ISOLATED_ROOT=$PWD +export BUCK_CLASSPATH_EXTRA + export BUCK_PLUGIN_RESOURCES=$(resolve $BUCK_PLUGIN_RESOURCES) export BUCK_PLUGIN_ROOT=$(resolve $BUCK_PLUGIN_ROOT) # necessary for "isolated buck" invocations to work correctly diff --git a/src/com/facebook/buck/util/env/BuckClasspath.java b/src/com/facebook/buck/util/env/BuckClasspath.java index b71d00fa930..f0ad260c76b 100644 --- a/src/com/facebook/buck/util/env/BuckClasspath.java +++ b/src/com/facebook/buck/util/env/BuckClasspath.java @@ -16,8 +16,10 @@ package com.facebook.buck.util.env; +import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; import java.io.File; import java.io.IOException; @@ -34,10 +36,17 @@ /** Current process classpath. Set by buckd or by launcher script. */ public class BuckClasspath { + /* + * Env values limit when spawning external process. + * https://github.com/torvalds/linux/blob/master/include/uapi/linux/limits.h#L8 + */ + public static final int ENV_ARG_MAX = 131000; + public static final String BOOTSTRAP_MAIN_CLASS = "com.facebook.buck.cli.bootstrapper.ClassLoaderBootstrapper"; public static final String ENV_VAR_NAME = "BUCK_CLASSPATH"; + public static final String EXTRA_ENV_VAR_NAME = "EXTRA_BUCK_CLASSPATH"; public static final String BOOTSTRAP_ENV_VAR_NAME = "CLASSPATH"; public static final String TEST_ENV_VAR_NAME = "BUCK_TEST_CLASSPATH_FILE"; @@ -151,4 +160,32 @@ private static ImmutableList readClasspaths(Path classpathsFile) throws IO .map(Path::toAbsolutePath) .collect(ImmutableList.toImmutableList()); } + + /** + * Generate env map containing buck classpath. Will split the classpath into an EXTRA_* env value + * if it exceeds the linux limit. + * + * @param classpath to be exported. + * @return map containing env variables. + */ + public static ImmutableMap getClasspathEnv(Iterable classpath) { + String arg = asArgument(classpath); + int limit = arg.length() > ENV_ARG_MAX ? arg.lastIndexOf(File.pathSeparator, ENV_ARG_MAX) : -1; + if (limit > 0 && limit < arg.length()) { + return ImmutableMap.of( + ENV_VAR_NAME, arg.substring(0, limit), + EXTRA_ENV_VAR_NAME, arg.substring(limit + 1)); + } + return ImmutableMap.of(ENV_VAR_NAME, arg); + } + + /** Generate env map containing buck bootstrap classpath. */ + public static ImmutableMap getBootstrapClasspathEnv( + ImmutableList bootstrapClasspath) { + return ImmutableMap.of(BOOTSTRAP_ENV_VAR_NAME, asArgument(bootstrapClasspath)); + } + + public static String asArgument(Iterable classpath) { + return Joiner.on(File.pathSeparator).join(classpath); + } } diff --git a/test/com/facebook/buck/cli/bootstrapper/ClassLoaderFactoryTest.java b/test/com/facebook/buck/cli/bootstrapper/ClassLoaderFactoryTest.java new file mode 100644 index 00000000000..1749f3671c0 --- /dev/null +++ b/test/com/facebook/buck/cli/bootstrapper/ClassLoaderFactoryTest.java @@ -0,0 +1,76 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.buck.cli.bootstrapper; + +import static org.hamcrest.Matchers.arrayWithSize; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.hasItemInArray; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.junit.MatcherAssert.assertThat; +import static org.hamcrest.object.HasToString.hasToString; +import static org.junit.Assert.assertThrows; + +import java.net.URL; +import java.net.URLClassLoader; +import java.util.HashMap; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; + +public class ClassLoaderFactoryTest { + + static final String CLASS_PATH_JAR = "/classPath.jar"; + static final String EXTRA_CLASS_PATH_JAR = "/extraClassPath.jar"; + + private final Map testEnvironment = new HashMap<>(); + private final ClassLoaderFactory classLoaderFactory = + new ClassLoaderFactory(testEnvironment::get); + + @Before + public void setUp() { + testEnvironment.clear(); + } + + @Test + public void testMissingBuckClassPlath() { + String expectedMessage = ClassLoaderFactory.BUCK_CLASSPATH + " not set"; + assertThrows(expectedMessage, RuntimeException.class, classLoaderFactory::create); + } + + @Test + public void testBuckClassPlath() { + testEnvironment.put(ClassLoaderFactory.BUCK_CLASSPATH, CLASS_PATH_JAR); + + URL[] urls = ((URLClassLoader) classLoaderFactory.create()).getURLs(); + + assertThat(urls, arrayWithSize(1)); + assertThat(urls, hasItemInArray(hasToString(containsString(CLASS_PATH_JAR)))); + assertThat(urls, not(hasItemInArray(hasToString(containsString(EXTRA_CLASS_PATH_JAR))))); + } + + @Test + public void testBuckClassPlathWithExtraClassPath() { + testEnvironment.put(ClassLoaderFactory.BUCK_CLASSPATH, CLASS_PATH_JAR); + testEnvironment.put(ClassLoaderFactory.EXTRA_BUCK_CLASSPATH, EXTRA_CLASS_PATH_JAR); + + URL[] urls = ((URLClassLoader) classLoaderFactory.create()).getURLs(); + + assertThat(urls, arrayWithSize(2)); + assertThat(urls, hasItemInArray(hasToString(containsString(CLASS_PATH_JAR)))); + assertThat(urls, hasItemInArray(hasToString(containsString(EXTRA_CLASS_PATH_JAR)))); + } +} diff --git a/test/com/facebook/buck/util/env/BuckClasspathTest.java b/test/com/facebook/buck/util/env/BuckClasspathTest.java index b704c70d05a..ce40bf1268f 100644 --- a/test/com/facebook/buck/util/env/BuckClasspathTest.java +++ b/test/com/facebook/buck/util/env/BuckClasspathTest.java @@ -16,15 +16,25 @@ package com.facebook.buck.util.env; +import static org.hamcrest.CoreMatchers.allOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.collection.IsMapContaining.hasEntry; +import static org.hamcrest.collection.IsMapContaining.hasKey; +import static org.hamcrest.core.StringContains.containsString; import static org.junit.Assume.assumeTrue; import com.facebook.buck.cli.bootstrapper.ClassLoaderBootstrapper; import com.facebook.buck.util.function.ThrowingFunction; import com.facebook.buck.util.stream.RichStream; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import java.net.URL; import java.net.URLClassLoader; import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Collections; import org.junit.Test; public class BuckClasspathTest { @@ -62,6 +72,33 @@ public void testBootstrapClasspathDoesNotHaveAccessToBuckFiles() throws Exceptio classLoader.loadClass(ImmutableList.class.getName()); } + @Test + public void testEnvHasBuckClasspath() { + String name = "testEnvHasBuckClasspath.jar"; + ImmutableMap env = + BuckClasspath.getClasspathEnv(Collections.singleton(Paths.get(name))); + + assertThat( + env, + allOf( + hasEntry(is(BuckClasspath.ENV_VAR_NAME), containsString(name)), + not(hasKey(is(BuckClasspath.EXTRA_ENV_VAR_NAME))))); + } + + @Test + public void testEnvHasBuckClasspathAndExtraClasspath() { + String name = "testEnvHasBuckClasspathAndExtraClasspath.jar"; + ImmutableMap env = + BuckClasspath.getClasspathEnv( + Collections.nCopies(BuckClasspath.ENV_ARG_MAX / name.length(), Paths.get(name))); + + assertThat( + env, + allOf( + hasEntry(is(BuckClasspath.ENV_VAR_NAME), containsString(name)), + hasEntry(is(BuckClasspath.EXTRA_ENV_VAR_NAME), containsString(name)))); + } + public URLClassLoader createClassLoader(ImmutableList classpath) { URL[] urls = RichStream.from(classpath) diff --git a/tools/ideabuck/src/com/facebook/buck/intellij/ideabuck/environment/EnvironmentFilter.java b/tools/ideabuck/src/com/facebook/buck/intellij/ideabuck/environment/EnvironmentFilter.java index 27a5f50a26e..da4e6b3e206 100644 --- a/tools/ideabuck/src/com/facebook/buck/intellij/ideabuck/environment/EnvironmentFilter.java +++ b/tools/ideabuck/src/com/facebook/buck/intellij/ideabuck/environment/EnvironmentFilter.java @@ -36,6 +36,7 @@ public class EnvironmentFilter { "Apple_PubSub_Socket_Render", // OS X pubsub control variable. "BUCK_BUILD_ID", // Build ID passed in from Python. "BUCK_CLASSPATH", // Main classpath; set in Python + "BUCK_CLASSPATH_EXTRA", // Main classpath; set in Python "CHGHG", // Mercurial "CHGTIMEOUT", // Mercurial "CLASSPATH", // Bootstrap classpath; set in Python.