Fix three high-impact Coverity defects (#13030)

bryancall · web-flow · commit 72ad8fbdc01b · 2026-03-30T14:54:31.000-07:00
* CID 1644226: Fix use-after-free in slice plugin — move
  TSfree() after last use of urlstr in DEBUG_LOG.

* CID 1644298: Fix OOB write in IpAllow — break after
  MAX_SUBJECTS exceeded instead of continuing.

* CID 1644219: Fix missing null terminator in slice plugin —
  replace strncpy() with memcpy() + explicit null terminator.

* Add gold tests for IpAllow subjects overflow and slice
  long ETag edge cases.
diff --git a/plugins/slice/server.cc b/plugins/slice/server.cc
@@ -26,6 +26,7 @@
 #include "ts/apidefs.h"
 #include "util.h"
 
+#include <algorithm>
 #include <cinttypes>
 
 namespace
@@ -463,14 +464,19 @@ handleNextServerHeader(Data *const data)
       data->m_blockstate = BlockState::PendingRef;
 
       // interior headers for new identifier reference
+      etaglen         = std::min(etaglen, static_cast<int>(sizeof(data->m_etag) - 1));
       data->m_etaglen = etaglen;
       if (0 < etaglen) {
-        strncpy(data->m_etag, etag, etaglen);
+        memcpy(data->m_etag, etag, etaglen);
       }
+      data->m_etag[etaglen] = '\0';
+
+      lastmodifiedlen         = std::min(lastmodifiedlen, static_cast<int>(sizeof(data->m_lastmodified) - 1));
       data->m_lastmodifiedlen = lastmodifiedlen;
       if (0 < lastmodifiedlen) {
-        strncpy(data->m_lastmodified, lastmodified, lastmodifiedlen);
+        memcpy(data->m_lastmodified, lastmodified, lastmodifiedlen);
       }
+      data->m_lastmodified[lastmodifiedlen] = '\0';
 
       // potentially new content length
       data->m_contentlen = blockcr.m_length;
diff --git a/plugins/slice/slice.cc b/plugins/slice/slice.cc
@@ -28,6 +28,7 @@
 #include "ts/ts.h"
 
 #include <charconv>
+#include <memory>
 #include <netinet/in.h>
 #include <array>
 #include <string_view>
@@ -50,14 +51,13 @@ TSCont global_read_resp_hdr_contp;
 static bool
 should_skip_this_obj(TSHttpTxn txnp, Config *const config)
 {
-  int         len    = 0;
-  char *const urlstr = TSHttpTxnEffectiveUrlStringGet(txnp, &len);
+  int                                      len = 0;
+  std::unique_ptr<char, decltype(&TSfree)> urlstr(TSHttpTxnEffectiveUrlStringGet(txnp, &len), TSfree);
 
-  bool const known_large = config->isKnownLargeObj({urlstr, static_cast<size_t>(len)});
-  TSfree(urlstr);
+  bool const known_large = config->isKnownLargeObj({urlstr.get(), static_cast<size_t>(len)});
 
   if (!known_large) {
-    DEBUG_LOG("Not a known large object, not slicing: %.*s", len, urlstr);
+    DEBUG_LOG("Not a known large object, not slicing: %.*s", len, urlstr.get());
     return true;
   }
 
diff --git a/src/proxy/IPAllow.cc b/src/proxy/IPAllow.cc
@@ -240,9 +240,10 @@ IpAllow::IpAllow(const char *ip_allow_config_var, const char *ip_categories_conf
     std::string_view::size_type s, e;
     for (s = 0, e = 0; s < subjects_sv.size() && e != subjects_sv.npos; s = e + 1) {
       e                           = subjects_sv.find(",", s);
-      std::string_view subject_sv = subjects_sv.substr(s, e);
+      std::string_view subject_sv = subjects_sv.substr(s, e == subjects_sv.npos ? subjects_sv.npos : e - s);
       if (i >= MAX_SUBJECTS) {
         Error("Too many ACL subjects were provided");
+        break;
       }
       if (subject_sv == "PEER") {
         subjects[i] = Subject::PEER;
diff --git a/tests/gold_tests/ip_allow/ip_allow_subjects.test.py b/tests/gold_tests/ip_allow/ip_allow_subjects.test.py
@@ -0,0 +1,30 @@
+'''
+Verify that exceeding MAX_SUBJECTS for proxy.config.acl.subjects logs an error and does not crash.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you 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.
+
+Test.Summary = '''
+Verify that exceeding MAX_SUBJECTS for proxy.config.acl.subjects logs an error and does not crash.
+'''
+
+# Scenario 1: Configuring 4 ACL subjects (more than MAX_SUBJECTS=3) should log
+# an error but not crash. Request should still succeed with 200 OK.
+Test.ATSReplayTest(replay_file="replay/ip_allow_subjects_overflow.replay.yaml")
+
+# Scenario 2: Configuring exactly 3 ACL subjects (equal to MAX_SUBJECTS) should
+# work without error. Request should succeed with 200 OK.
+Test.ATSReplayTest(replay_file="replay/ip_allow_subjects_valid.replay.yaml")
diff --git a/tests/gold_tests/ip_allow/replay/ip_allow_subjects_overflow.replay.yaml b/tests/gold_tests/ip_allow/replay/ip_allow_subjects_overflow.replay.yaml
@@ -0,0 +1,72 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you 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.
+
+meta:
+  version: "1.0"
+
+autest:
+  description: 'Verify that exceeding MAX_SUBJECTS (4 subjects) logs an error but does not crash'
+
+  server:
+    name: 'server-overflow'
+
+  client:
+    name: 'client-overflow'
+
+  ats:
+    name: 'ts-overflow'
+
+    process_config:
+      enable_cache: false
+      disable_log_checks: true
+
+    records_config:
+      proxy.config.acl.subjects: 'PEER,PROXY,PLUGIN,PEER'
+
+    remap_config:
+      - from: "http://www.example.com/"
+        to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+
+    log_validation:
+      diags_log:
+        contains:
+          - expression: "Too many ACL subjects were provided"
+            description: "Should log error when more than MAX_SUBJECTS are configured"
+
+sessions:
+- transactions:
+
+  - client-request:
+      method: GET
+      url: /get
+      version: '1.1'
+      headers:
+        fields:
+        - [Host, www.example.com]
+        - [uuid, overflow-subjects-request]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [Content-Length, "3"]
+        - [Connection, close]
+      content:
+        data: "xxx"
+
+    proxy-response:
+      status: 200
diff --git a/tests/gold_tests/ip_allow/replay/ip_allow_subjects_valid.replay.yaml b/tests/gold_tests/ip_allow/replay/ip_allow_subjects_valid.replay.yaml
@@ -0,0 +1,71 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you 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.
+
+meta:
+  version: "1.0"
+
+autest:
+  description: 'Verify that exactly MAX_SUBJECTS (3 subjects) works without error'
+
+  server:
+    name: 'server-valid'
+
+  client:
+    name: 'client-valid'
+
+  ats:
+    name: 'ts-valid'
+
+    process_config:
+      enable_cache: false
+
+    records_config:
+      proxy.config.acl.subjects: 'PEER,PROXY,PLUGIN'
+
+    remap_config:
+      - from: "http://www.example.com/"
+        to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+
+    log_validation:
+      diags_log:
+        excludes:
+          - expression: "Too many ACL subjects were provided"
+            description: "Should not log error when exactly MAX_SUBJECTS are configured"
+
+sessions:
+- transactions:
+
+  - client-request:
+      method: GET
+      url: /get
+      version: '1.1'
+      headers:
+        fields:
+        - [Host, www.example.com]
+        - [uuid, valid-subjects-request]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [Content-Length, "3"]
+        - [Connection, close]
+      content:
+        data: "xxx"
+
+    proxy-response:
+      status: 200
diff --git a/tests/gold_tests/pluginTest/slice/replay/slice_long_etag.replay.yaml b/tests/gold_tests/pluginTest/slice/replay/slice_long_etag.replay.yaml
@@ -0,0 +1,111 @@
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you 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.
+
+meta:
+  version: "1.0"
+
+autest:
+  description: 'Verify slice plugin handles long ETag values without crashing'
+
+  server:
+    name: 'server-long-etag'
+    process_config:
+      other_args: '-f "{url}"'
+
+  client:
+    name: 'client-long-etag'
+
+  ats:
+    name: 'ts-long-etag'
+
+    process_config:
+      enable_cache: true
+
+    records_config:
+      proxy.config.diags.debug.enabled: 1
+      proxy.config.diags.debug.tags: 'slice'
+
+    remap_config:
+      - from: "http://preload/"
+        to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+
+      - from: "http://slice/"
+        to: "http://127.0.0.1:{SERVER_HTTP_PORT}/"
+        plugins:
+          - name: "slice.so"
+            args:
+              - "--blockbytes-test=11"
+
+# Body is 28 bytes ("lets go surfin now everybody"), block size is 11.
+# This means 3 blocks: bytes 0-10, 11-21, 22-27.
+# The slice plugin copies the etag from block 0 into data->m_etag
+# for validation on blocks 1 and 2. A 4000-char etag exercises the
+# null-termination and bounds-checking code path.
+
+sessions:
+
+# Session 1: Preload the asset into cache (bypassing slice plugin)
+- transactions:
+
+  - client-request:
+      method: GET
+      url: /long_etag
+      version: '1.1'
+      headers:
+        fields:
+        - [Host, preload]
+        - [uuid, preload-long-etag]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [Content-Length, "28"]
+        - [Connection, close]
+        - [Cache-Control, "max-age=500"]
+        - [Etag, '"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"']
+        - [Last-Modified, "Fri, 07 Mar 2025 18:06:58 GMT"]
+      content:
+        data: "lets go surfin now everybody"
+
+    proxy-response:
+      status: 200
+
+# Session 2: Fetch through slice plugin (should serve from cache in blocks)
+- transactions:
+
+  - client-request:
+      delay: 100ms
+      method: GET
+      url: /long_etag
+      version: '1.1'
+      headers:
+        fields:
+        - [Host, slice]
+        - [uuid, slice-long-etag]
+
+    server-response:
+      status: 200
+      reason: OK
+      headers:
+        fields:
+        - [Content-Length, "28"]
+      content:
+        data: "lets go surfin now everybody"
+
+    proxy-response:
+      status: 200
diff --git a/tests/gold_tests/pluginTest/slice/slice_long_etag.test.py b/tests/gold_tests/pluginTest/slice/slice_long_etag.test.py
@@ -0,0 +1,27 @@
+'''
+Verify slice plugin handles long ETag values without crashing.
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you 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.
+
+Test.Summary = '''
+Verify the slice plugin handles long ETag values (4000 chars) without crashing
+when copying between internal buffers during multi-block range requests.
+'''
+
+Test.SkipUnless(Condition.PluginExists('slice.so'),)
+
+Test.ATSReplayTest(replay_file="replay/slice_long_etag.replay.yaml")
