kennknowles commented on code in PR #40008:
URL: https://github.com/apache/beam/pull/40008#discussion_r4186011977
##########
examples/java/src/main/java/org/apache/beam/examples/subprocess/utils/ExecutableFile.java:
##########
@@ -26,6 +27,11 @@
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should be `final`.
##########
it/google-cloud-platform/src/main/java/org/apache/beam/it/gcp/bigtable/BigtableResourceManager.java:
##########
@@ -77,6 +78,11 @@
*
* <p>The class is thread-safe.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/BoundedTrieData.java:
##########
@@ -57,8 +57,12 @@
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
@SuppressFBWarnings(
- value = "IS2_INCONSISTENT_SYNC",
- justification = "Some access on purpose are left unsynchronized")
+ value = {"IS2_INCONSISTENT_SYNC", "CT_CONSTRUCTOR_THROW"},
+ justification =
+ "Some access on purpose are left unsynchronized."
+ + " Public, so it cannot be made final despite @Internal."
+ + " Out-of-tree code such as a forked runner may already subclass
it."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/2.0/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/io/UnboundedSourceWrapper.java:
##########
@@ -67,6 +68,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/io/source/LazyFlinkSourceSplitEnumerator.java:
##########
@@ -34,6 +35,11 @@
import org.slf4j.LoggerFactory;
/** Splits a bounded Beam source and assigns one split for each reader
request. */
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/FlinkStreamingPortablePipelineTranslator.java:
##########
@@ -130,6 +131,11 @@
"keyfor",
"nullness"
}) // TODO(https://github.com/apache/beam/issues/20497)
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
it/clickhouse/src/main/java/org/apache/beam/it/clickhouse/ClickHouseResourceManager.java:
##########
@@ -44,6 +45,11 @@
*
* <p>The class is thread-safe.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/io/UnboundedSourceWrapper.java:
##########
@@ -66,6 +67,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/sink/CsvSink.java:
##########
@@ -36,6 +37,11 @@
* "spark.metrics.conf.*.sink.csv.unit"=seconds
* }</pre>
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkBeamMetricSource.java:
##########
@@ -18,12 +18,18 @@
package org.apache.beam.runners.spark.metrics;
import com.codahale.metrics.MetricRegistry;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.spark.metrics.source.Source;
/**
* A Spark {@link Source} that is tailored to expose a {@link
SparkBeamMetric}, wrapping an
* underlying {@link org.apache.beam.sdk.metrics.MetricResults} instance.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/structuredstreaming/metrics/sink/CodahaleCsvSink.java:
##########
@@ -36,6 +37,11 @@
* "spark.metrics.conf.*.sink.csv.unit"=seconds
* }</pre>
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/sink/GraphiteSink.java:
##########
@@ -39,6 +40,11 @@
*
"spark.metrics.conf.*.sink.graphite.regex"="<optional_regex_to_send_matching_metrics>"
* }</pre>
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/jmh/src/main/java/org/apache/beam/sdk/jmh/schemas/RowBundle.java:
##########
@@ -54,6 +55,11 @@
* adequately timestamped without risking generating wrong results.
*/
@State(Scope.Benchmark)
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Subclassed inside Beam, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
The jmh folder is just our own benchmarks (and TBH not the main ones we look
at much). These can be tweaked in breaking ways also.
This class in particular would benefit from a refactor so the throwing can
happen in a static factory method, and then "just data" passed to the
constructor.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/providers/JavaRowUdf.java:
##########
@@ -58,6 +58,11 @@
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.ImmutableMap;
import
org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.io.ByteStreams;
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
Definitely not meant for subclassing
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/join/CoGbkResult.java:
##########
@@ -55,6 +56,11 @@
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one is also only meant to be consumed by users, not extended or
produced. Nonetheless it might be extended somewhere out the wild.
##########
examples/java/src/main/java/org/apache/beam/examples/subprocess/utils/CallingSubProcessUtils.java:
##########
@@ -88,6 +89,11 @@ private static void releaseSemaphore(String binaryName)
throws IllegalStateExcep
}
/** Permit class for access to worker cpu resources. */
+ @SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on
the classpath.")
Review Comment:
This one can/should be `final`.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ExplicitShardedFile.java:
##########
@@ -38,6 +39,12 @@
/** A sharded file where the file names are simply provided. */
@Internal
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public, so it cannot be made final despite @Internal."
+ + " Out-of-tree code such as a forked runner may already subclass
it."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
the whole `util` folder is not public
##########
it/splunk/src/main/java/org/apache/beam/it/splunk/SplunkResourceManager.java:
##########
@@ -60,6 +61,11 @@
* <p>Note: The Splunk TestContainer will only run on M1 Mac's if the Docker
version is >= 4.16.0
* and the "Use Rosetta for x86/amd64 emulation on Apple Silicon" setting is
enabled.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/2.0/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/state/FlinkStateInternals.java:
##########
@@ -103,6 +104,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/construction/SerializablePipelineOptions.java:
##########
@@ -32,6 +33,11 @@
* Holds a {@link PipelineOptions} in JSON serialized form and calls {@link
* FileSystems#setDefaultPipelineOptions(PipelineOptions)} on construction or
on deserialization.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/construction/graph/QueryablePipeline.java:
##########
@@ -56,6 +57,11 @@
* other.
*/
@SuppressWarnings({"nullness", "keyfor"}) //
TODO(https://github.com/apache/beam/issues/20497)
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API for runner authors, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
the whole `util` folder is not public
##########
examples/java/src/main/java/org/apache/beam/examples/complete/datatokenization/utils/SchemasUtils.java:
##########
@@ -53,6 +54,11 @@
"argument",
"return"
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should be `final`.
##########
runners/flink/2.0/src/main/java/org/apache/beam/runners/flink/FlinkStreamingPortablePipelineTranslator.java:
##########
@@ -130,6 +131,11 @@
"keyfor",
"nullness"
}) // TODO(https://github.com/apache/beam/issues/20497)
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/core-java/src/main/java/org/apache/beam/runners/core/StatefulDoFnRunner.java:
##########
@@ -59,6 +60,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
Note that _everything_ under the "runners" folder is non-public.
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionViews.java:
##########
@@ -473,6 +474,12 @@ public static Map<TupleTag<?>, PValue>
toAdditionalInputs(Iterable<PCollectionVi
* <p>{@link SingletonViewFn} is meant to be removed in the future and
replaced with this class.
*/
@Internal
+ @SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public, so it cannot be made final despite @Internal."
+ + " Out-of-tree code such as a forked runner may already
subclass it."
+ + " A finalizer attack needs an attacker-supplied subclass on
the classpath.")
Review Comment:
the whole `util` folder is not public
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/io/source/FlinkSourceSplitEnumerator.java:
##########
@@ -39,6 +40,11 @@
import org.slf4j.LoggerFactory;
/** Splits a Beam source and assigns its splits to Flink source readers
round-robin. */
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/stableinput/KeyedBufferingElementsHandler.java:
##########
@@ -31,6 +32,11 @@
@SuppressWarnings({
"rawtypes" // TODO(https://github.com/apache/beam/issues/20447)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/stableinput/BufferingDoFnRunner.java:
##########
@@ -55,6 +56,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java:
##########
@@ -96,6 +97,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/jet/src/main/java/org/apache/beam/runners/jet/JetRunner.java:
##########
@@ -47,6 +48,11 @@
import org.slf4j.LoggerFactory;
/** Jet specific implementation of Beam's {@link PipelineRunner}. */
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/translation/wrappers/streaming/state/FlinkStateInternals.java:
##########
@@ -103,6 +104,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/jet/src/main/java/org/apache/beam/runners/jet/processors/AssignWindowP.java:
##########
@@ -43,6 +44,11 @@
"nullness",
"keyfor"
}) // TODO(https://github.com/apache/beam/issues/20497)
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/structuredstreaming/metrics/sink/CodahaleGraphiteSink.java:
##########
@@ -39,6 +40,11 @@
*
"spark.metrics.conf.*.sink.graphite.regex"="<optional_regex_to_send_matching_metrics>"
* }</pre>
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/translation/SparkDatasetPortablePipelineTranslator.java:
##########
@@ -81,6 +82,11 @@
"unchecked",
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/translation/SparkBatchPortablePipelineTranslator.java:
##########
@@ -77,6 +78,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/structuredstreaming/metrics/SparkBeamMetricSource.java:
##########
@@ -18,12 +18,18 @@
package org.apache.beam.runners.spark.structuredstreaming.metrics;
import com.codahale.metrics.MetricRegistry;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.spark.metrics.source.Source;
/**
* A Spark {@link Source} that is tailored to expose a {@link
SparkBeamMetric}, wrapping an
* underlying {@link org.apache.beam.sdk.metrics.MetricResults} instance.
*/
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/CompressedSource.java:
##########
@@ -65,6 +66,11 @@
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one is public, but very unlikely to be subclasses, and the private
constructor can not be invoked. So it is safe to change it to a factory method
that would throw the exception.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/fn/data/BeamFnDataOutboundAggregator.java:
##########
@@ -61,6 +62,11 @@
// create another memory barrier. Also note that flush is always invoked when
synchronizing on
// flushLock when there is a periodic flushing thread.
@NotThreadSafe
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Subclassed inside Beam, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
Everything in the `fn` directory is also not public API and can be changed
and refactored to fix it, e.g. with a static factory method.
##########
runners/spark/src/main/java/org/apache/beam/runners/spark/translation/SparkStreamingPortablePipelineTranslator.java:
##########
@@ -72,6 +73,11 @@
"rawtypes", // TODO(https://github.com/apache/beam/issues/20447)
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/ConvertHelpers.java:
##########
@@ -69,6 +70,11 @@ private static class SchemaInformationProviders {
private static final Object lock = new Object();
/** Return value after converting a schema. */
+ @SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on
the classpath.")
Review Comment:
anything in a directory like 'util' or 'utils' is meant to not be public.
Certainly not this class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Sample.java:
##########
@@ -291,6 +292,11 @@ public T extractOutput(List<T> accumulator) {
*
* @param <T> the type of the elements
*/
+ @SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on
the classpath.")
Review Comment:
This one can/should (probably) be `final`. I didn't actually check if
anything extends it, but it does not appear to be a deliberate base class or
interface class.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/construction/DefaultArtifactResolver.java:
##########
@@ -37,6 +38,11 @@
@SuppressWarnings({
"nullness" // TODO(https://github.com/apache/beam/issues/20497)
})
+@SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public API for runner authors, so it cannot be made final."
+ + " A finalizer attack needs an attacker-supplied subclass on the
classpath.")
Review Comment:
the whole `util` folder is not public
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionViews.java:
##########
@@ -574,6 +581,12 @@ public interface IsSingletonView<T> {}
* @deprecated See {@link SingletonViewFn2}.
*/
@Deprecated
+ @SuppressFBWarnings(
+ value = "CT_CONSTRUCTOR_THROW",
+ justification =
+ "Public, so it cannot be made final despite @Internal."
+ + " Out-of-tree code such as a forked runner may already
subclass it."
+ + " A finalizer attack needs an attacker-supplied subclass on
the classpath.")
Review Comment:
the whole `util` folder is not public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
