Copilot commented on code in PR #12723:
URL: https://github.com/apache/trafficserver/pull/12723#discussion_r2587264724
##########
tools/http_load/http_load.c:
##########
@@ -648,18 +648,43 @@ read_url_file(char *url_file)
constructed by the URL host and possibly port (if not port 80) */
char hdr_buf[2048];
- int hdr_bytes = 0;
- hdr_bytes += snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"User-Agent: %s\r\n", user_agent);
- if (cookie)
- hdr_bytes += snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"Cookie: %s\r\n", cookie);
- if (do_accept_gzip)
- hdr_bytes += snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"Accept-Encoding: gzip\r\n");
+ int hdr_bytes = 0;
+ int n;
+
+ n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes, "User-Agent:
%s\r\n", user_agent);
+ if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
+ hdr_bytes += n;
+ }
+
+ if (cookie) {
+ n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes, "Cookie:
%s\r\n", cookie);
+ if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
+ hdr_bytes += n;
+ }
+ }
+
+ if (do_accept_gzip) {
+ n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"Accept-Encoding: gzip\r\n");
+ if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
+ hdr_bytes += n;
+ }
+ }
+
/* Add Connection: keep-alive header if keep_alive requested, and version !=
"1.1" */
- if ((keep_alive > 0) && !is_http_1_1)
- hdr_bytes += snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"Connection: keep-alive\r\n");
+ if ((keep_alive > 0) && !is_http_1_1) {
+ n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"Connection: keep-alive\r\n");
+ if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
+ hdr_bytes += n;
+ }
+ }
+
if (extra_headers != NULL) {
- hdr_bytes += snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes,
"%s\r\n", extra_headers);
+ n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes, "%s\r\n",
extra_headers);
+ if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
+ hdr_bytes += n;
+ }
}
+
snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes, "\r\n");
Review Comment:
This snprintf call should also check its return value before using the
result, consistent with the other fixes in this PR. If the buffer is already
full from previous headers, this could write beyond the buffer boundary.
```suggestion
n = snprintf(&hdr_buf[hdr_bytes], sizeof(hdr_buf) - hdr_bytes, "\r\n");
if (n > 0 && (size_t)n < sizeof(hdr_buf) - hdr_bytes) {
hdr_bytes += n;
}
```
##########
tools/http_load/http_load.c:
##########
@@ -725,20 +750,36 @@ read_url_file(char *url_file)
req_bytes = snprintf(req_buf, sizeof(req_buf), "GET %.500s HTTP/%s\r\n",
urls[num_urls].filename, http_version);
if (extra_headers == NULL || !strstr(extra_headers, "Host:")) {
- if (urls[num_urls].port != 80)
- req_bytes += snprintf(&req_buf[req_bytes], sizeof(req_buf) -
req_bytes, "Host: %s:%d\r\n", urls[num_urls].hostname,
- urls[num_urls].port);
- else
- req_bytes += snprintf(&req_buf[req_bytes], sizeof(req_buf) -
req_bytes, "Host: %s\r\n", urls[num_urls].hostname);
+ if (urls[num_urls].port != 80) {
+ n = snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes, "Host:
%s:%d\r\n", urls[num_urls].hostname,
+ urls[num_urls].port);
+ if (n > 0 && (size_t)n < sizeof(req_buf) - req_bytes) {
+ req_bytes += n;
+ }
+ } else {
+ n = snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes, "Host:
%s\r\n", urls[num_urls].hostname);
+ if (n > 0 && (size_t)n < sizeof(req_buf) - req_bytes) {
+ req_bytes += n;
+ }
+ }
}
if (unique_id == 1) {
- req_bytes += snprintf(&req_buf[req_bytes],
sizeof(req_buf) - req_bytes, "X-ID: ");
- urls[num_urls].unique_id_offset = req_bytes;
- req_bytes += snprintf(&req_buf[req_bytes],
sizeof(req_buf) - req_bytes, "%09u\r\n", 0);
+ n = snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes, "X-ID: ");
+ if (n > 0 && (size_t)n < sizeof(req_buf) - req_bytes) {
+ req_bytes += n;
+ urls[num_urls].unique_id_offset = req_bytes;
+ n = snprintf(&req_buf[req_bytes],
sizeof(req_buf) - req_bytes, "%09u\r\n", 0);
+ if (n > 0 && (size_t)n < sizeof(req_buf) - req_bytes) {
+ req_bytes += n;
+ }
+ }
}
// add the common hdr here
- req_bytes += snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes,
hdr_buf, 0);
+ n = snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes, hdr_buf, 0);
Review Comment:
Using hdr_buf as a format string with snprintf is unsafe. If extra_headers
contains format specifiers like %s or %n, this could lead to format string
vulnerabilities or crashes. The code should use a safe string copy method
instead, or if using snprintf, should use "%s" as the format string with
hdr_buf as the argument. Additionally, the extra parameter `0` appears to be
incorrect and doesn't match any format specifier.
```suggestion
n = snprintf(&req_buf[req_bytes], sizeof(req_buf) - req_bytes, "%s",
hdr_buf);
```
##########
tools/http_load/http_load.c:
##########
@@ -1,6 +1,6 @@
/* http_load - multiprocessing http test client
**
-** Copyright � 1998,1999,2001 by Jef Poskanzer <[email protected]>.
+** Copyright � 1998,1999,2001 by Jef Poskanzer <[email protected]>.
Review Comment:
The copyright symbol appears to have been changed from © to �. This looks
like an unintended character encoding corruption rather than an intentional
change. Please revert this change to maintain the original copyright symbol.
```suggestion
** Copyright © 1998,1999,2001 by Jef Poskanzer <[email protected]>.
```
--
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]
