Dennis-Mircea commented on code in PR #1181:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1181#discussion_r3802440251
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/state/KubernetesAutoScalerStateStore.java:
##########
@@ -388,15 +394,31 @@ private static String decompress(String compressed) {
try {
byte[] bytes = Base64.getDecoder().decode(compressed);
try (var zi = new GZIPInputStream(new
ByteArrayInputStream(bytes))) {
- return IOUtils.toString(zi, StandardCharsets.UTF_8);
+ return readBounded(zi, MAX_DECOMPRESSED_BYTES);
}
} catch (Exception e) {
- LOG.warn("Error while decompressing scaling data, treating as
uncompressed");
+ LOG.warn("Error while decompressing scaling data, treating as
uncompressed", e);
Review Comment:
This log also fires on the normal migration path. Values stored before
compression are not valid gzip, so `GZIPInputStream` throws and the code
correctly falls back to treating them as uncompressed. Logging a full stack
trace at WARN for that expected case is noise. Keep it without the exception,
or attach it only when the value was expected to be compressed.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/autoscaler/state/KubernetesAutoScalerStateStore.java:
##########
@@ -80,6 +80,12 @@ public class KubernetesAutoScalerStateStore
@VisibleForTesting protected static final int MAX_CM_BYTES = 1000000;
+ /* Caps the size of a single decompressed value so that a
crafted/corrupted ConfigMap
+ * entry (e.g. a gzip bomb) cannot exhaust operator memory during
decompression. Matches
+ * the YAML loader's code point limit below, since larger content would be
rejected by
+ * the YAML parser anyway. */
+ @VisibleForTesting protected static final int MAX_DECOMPRESSED_BYTES = 20
* 1024 * 1024;
Review Comment:
20MB matches the YAML code point limit, not the heap. A legitimate value
decompresses to at most ~4MB, because the compressed form is capped near 1MB
(the ConfigMap limit plus the `MAX_CM_BYTES` trim) and the operator's own YAML
compresses only ~2.5x to 4x (measured on real metric history), whereas a
crafted or hijacked payload reaches its theoretical ~1032:1 maximum, which is
the whole reason for a cap.
The default operator sets no heap limit, the peak per decode is ~32MB
(ByteArrayOutputStream overshoot), and the cap bounds one value rather than the
total, so concurrent poisoned CMs across the reconcile pool can still OOM under
it. Deriving it from `Runtime.maxMemory()`, or dropping it near the ~4MB real
ceiling, fits better than the parser limit.
--
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]
