Copilot commented on code in PR #13838:
URL: https://github.com/apache/trafficserver/pull/13838#discussion_r4476563021
##########
src/api/InkAPI.cc:
##########
@@ -9715,7 +9715,8 @@ TSLogStringUnmarshal(char **buf, char *dest, int len)
// of the alignment.
if (int l = strlen(*buf); l < len) {
memcpy(dest, *buf, l);
- return {l, l};
+ // TSLogStringMarshal also wrote the terminating NUL, so the field
occupies one more byte than was written to dest.
+ return {l + 1, l};
} else {
Review Comment:
`strlen(*buf)` can read past the provided `len` before the `l < len` check
runs, which is an out-of-bounds read if the log buffer is malformed/corrupted
or missing a terminating NUL within `len`. Prefer a bounded length computation
(e.g., `strnlen(*buf, len)` or `memchr` for `\\0`) and treat a missing NUL
within `len` as an error. Optionally, also NUL-terminate `dest` when space
permits to make the destination buffer safer for any downstream C-string usage.
##########
tests/uranium_tests/pluginTest/jax_fingerprint/tls_partial_client.py:
##########
@@ -0,0 +1,96 @@
+#!/usr/bin/env python3
+'''Complete a TLS handshake with ATS, optionally send a partial request, and
wait for ATS to close.'''
+# 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.
+
+import argparse
+import socket
+import ssl
+import sys
+
+
+def parse_args() -> argparse.Namespace:
+ parser = argparse.ArgumentParser(description=__doc__)
+ parser.add_argument('port', type=int, help='The ATS TLS port.')
+ parser.add_argument('sni', help='The SNI to send in the ClientHello.')
+ parser.add_argument('--path', help='Send an incomplete request header for
this path. Without it, send nothing.')
+ parser.add_argument(
+ '--complete-first',
+ metavar='UUID',
+ help='Before the incomplete request, send a complete keep-alive
request with this uuid and read its response.')
+ parser.add_argument('--timeout', type=float, default=30.0, help='Seconds
to wait for ATS to close the connection.')
+ return parser.parse_args()
+
+
+def read_response(tls_sock: ssl.SSLSocket) -> bytes:
+ """Read one response whose body length is given by Content-Length.
+
+ :param tls_sock: The connection to read from.
+ :returns: The response header block and body.
+ """
+ data = b''
+ while b'\r\n\r\n' not in data:
+ chunk = tls_sock.recv(4096)
+ if not chunk:
+ raise ConnectionError('ATS closed the connection before sending a
complete response.')
+ data += chunk
+ header, _, body = data.partition(b'\r\n\r\n')
Review Comment:
Using `data += chunk` / `body += chunk` repeatedly can devolve into
quadratic-time copying for larger responses. Even though this is a test helper,
it's easy to avoid by using a `bytearray` and `.extend()` (then `bytes()` at
the end), which keeps the helper robust if response sizes change.
##########
tests/uranium_tests/pluginTest/jax_fingerprint/tls_partial_client.py:
##########
@@ -0,0 +1,96 @@
+#!/usr/bin/env python3
+'''Complete a TLS handshake with ATS, optionally send a partial request, and
wait for ATS to close.'''
+# 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.
+
+import argparse
+import socket
+import ssl
+import sys
+
+
+def parse_args() -> argparse.Namespace:
+ parser = argparse.ArgumentParser(description=__doc__)
+ parser.add_argument('port', type=int, help='The ATS TLS port.')
+ parser.add_argument('sni', help='The SNI to send in the ClientHello.')
+ parser.add_argument('--path', help='Send an incomplete request header for
this path. Without it, send nothing.')
+ parser.add_argument(
+ '--complete-first',
+ metavar='UUID',
+ help='Before the incomplete request, send a complete keep-alive
request with this uuid and read its response.')
+ parser.add_argument('--timeout', type=float, default=30.0, help='Seconds
to wait for ATS to close the connection.')
+ return parser.parse_args()
+
+
+def read_response(tls_sock: ssl.SSLSocket) -> bytes:
+ """Read one response whose body length is given by Content-Length.
+
+ :param tls_sock: The connection to read from.
+ :returns: The response header block and body.
+ """
+ data = b''
+ while b'\r\n\r\n' not in data:
+ chunk = tls_sock.recv(4096)
+ if not chunk:
+ raise ConnectionError('ATS closed the connection before sending a
complete response.')
+ data += chunk
+ header, _, body = data.partition(b'\r\n\r\n')
+ content_length = 0
+ for line in header.split(b'\r\n')[1:]:
+ name, _, value = line.partition(b':')
+ if name.strip().lower() == b'content-length':
+ content_length = int(value.strip())
+ while len(body) < content_length:
+ chunk = tls_sock.recv(4096)
+ if not chunk:
+ raise ConnectionError('ATS closed the connection before sending
the response body.')
+ body += chunk
Review Comment:
Using `data += chunk` / `body += chunk` repeatedly can devolve into
quadratic-time copying for larger responses. Even though this is a test helper,
it's easy to avoid by using a `bytearray` and `.extend()` (then `bytes()` at
the end), which keeps the helper robust if response sizes change.
--
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]
