You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/11/10 10:10:53 UTC
[GitHub] [trafficserver] bryancall opened a new pull request, #9189: Removed most sprintf and ignore sprintf warnings in bind code
bryancall opened a new pull request, #9189:
URL: https://github.com/apache/trafficserver/pull/9189
When compiling on the mac I am getting a bunch of compiler errors for sprintf. I converted all of them to snprintf, except for the bind code that we copied over. For that I am ignoring the errors.
@bneradt I changed the way I am filling the `resize()` in `TransactionData::initialize_default_sensitive_field`. Hopefully this won't cause any issues. My worry is that we are writing 9 bytes including the null into data. From my understanding it is not guaranteed to have space for that.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] moonchen commented on pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
moonchen commented on PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#issuecomment-1312161873
A lot of nice changes. Thanks!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] moonchen commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
moonchen commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1020535139
##########
proxy/http/remap/unit-tests/plugin_testing_common.cc:
##########
@@ -46,7 +46,7 @@ getTemporaryDir()
tmpDir /= "sandbox_XXXXXX";
char dirNameTemplate[tmpDir.string().length() + 1];
- sprintf(dirNameTemplate, "%s", tmpDir.c_str());
+ snprintf(dirNameTemplate, sizeof(dirNameTemplate), "%s", tmpDir.c_str());
Review Comment:
A lot of nice changes. Thanks!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1019557629
##########
plugins/experimental/ja3_fingerprint/ja3_fingerprint.cc:
##########
@@ -319,7 +319,7 @@ client_hello_ja3_handler(TSCont contp, TSEvent event, void *edata)
MD5((unsigned char *)data->ja3_string.c_str(), data->ja3_string.length(), digest);
for (int i = 0; i < 16; i++) {
- sprintf(&(data->md5_string[i * 2]), "%02x", static_cast<unsigned int>(digest[i]));
+ snprintf(&(data->md5_string[i * 2]), 3, "%02x", static_cast<unsigned int>(digest[i]));
Review Comment:
It seems to me that relying on 3 working isn't much different than just using sprintf because you know thinking through it that you won't overrun the buffer. Let's use the buffer size for snprintf instead:
1. Define a constexpr for the buffer size: `constexpr int MD5_BUFSIZE = 33;`
2. Use that in the buffer size declaration above: `char md5_string[MD5_BUFSIZE];`
3. Here, use the known buffer size: `snprintf(&(data->md5_string[i * 2]), MD5_BUFSIZE - (i * 2), "%02x", static_cast<unsigned int>(digest[i]));`
Alternatively to defining `MD5_BUFSIZE`, `sizeof(data->md5_string)` could be used as well.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bryancall commented on PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#issuecomment-1312001647
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall merged pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bryancall merged PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bryancall commented on PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#issuecomment-1311783609
[approve ci]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bryancall commented on PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#issuecomment-1310579621
[approve ci clang-format]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1019359223
##########
plugins/experimental/traffic_dump/transaction_data.cc:
##########
@@ -106,16 +106,9 @@ TransactionData::response_buffer_handler(TSCont contp, TSEvent event, void *edat
void
TransactionData::initialize_default_sensitive_field()
{
- // 128 KB is the maximum size supported for all headers, so this size should
- // be plenty large for our needs.
+ // 128 KB is the maximum size supported for all headers, so this size should be plenty large for our needs.
constexpr size_t default_field_size = 128 * 1024;
- default_sensitive_field_value.resize(default_field_size);
-
- char *field_buffer = default_sensitive_field_value.data();
- for (auto i = 0u; i < default_field_size; i += 8) {
- sprintf(field_buffer, "%07x ", i / 8);
- field_buffer += 8;
- }
Review Comment:
I appreciate transitioning away from sprintf, but we should keep the original value rather than changing to all 'x's. Can we change to the following:
```
void
TransactionData::initialize_default_sensitive_field()
{
std::ostringstream ss;
// 128 KB is the maximum size supported for all headers, so this size should
// be plenty large for our needs.
constexpr size_t default_field_size = 128 * 1024;
for (auto i = 0u; i < default_field_size; i += 8) {
ss << std::hex << std::setw(7) << std::setfill('0') << i / 8 << " ";
}
default_sensitive_field_value = ss.str();
}
```
Note that there is no performance concern here: this function is called once on TSPluginInit and not after that. The above is correct and safe.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#issuecomment-1310668586
> @bneradt I changed the way I am filling the `resize()` in `TransactionData::initialize_default_sensitive_field`. Hopefully this won't cause any issues. My worry is that we are writing 9 bytes including the null into data. From my understanding it is not guaranteed to have space for that.
In case it's helpful or interesting, the original was safe. Since C++11, std::string is guaranteed to have a null terminator. To demonstrate this, let's think of the default string being a size of 8 characters instead of 128k to make it easier to think through. Then the function becomes:
```cpp
// 128 KB is the maximum size supported for all headers, so this size should
// be plenty large for our needs.
//constexpr size_t default_field_size = 128 * 1024;
constexpr size_t default_field_size = 8; // <--- Changed to 8.
default_sensitive_field_value.resize(default_field_size);
char *field_buffer = default_sensitive_field_value.data();
for (auto i = 0u; i < default_field_size; i += 8) {
sprintf(field_buffer, "%07x ", i / 8);
field_buffer += 8;
}
```
Thus the internal buffer size has to be at least 9 (8 for the resize value for the number of characters, and then one more value for the null terminator). The for loop will execute exactly once and write 8 characters (7 `0` values and a space "` `" character) and then sprintf will null terminate the string at `default_sensitive_field_value[8]` which is already guaranteed to be null. Thus there is no buffer overrun in the original and its behavior should be correct.
That said, I prefer the ostringstream version I suggest in my comment over the sprintf.
Again, thank you for the PR. I'm happier with the update to the function.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1019557629
##########
plugins/experimental/ja3_fingerprint/ja3_fingerprint.cc:
##########
@@ -319,7 +319,7 @@ client_hello_ja3_handler(TSCont contp, TSEvent event, void *edata)
MD5((unsigned char *)data->ja3_string.c_str(), data->ja3_string.length(), digest);
for (int i = 0; i < 16; i++) {
- sprintf(&(data->md5_string[i * 2]), "%02x", static_cast<unsigned int>(digest[i]));
+ snprintf(&(data->md5_string[i * 2]), 3, "%02x", static_cast<unsigned int>(digest[i]));
Review Comment:
It seems to me that relying on 3 working isn't much different than just using sprintf because you know thinking through it that you won't overrun the buffer. Let's use the buffer size for snprintf instead:
1. Define a constexpr for the buffer size: `constexpr int MD5_BUFSIZE = 33;`
2. Use that in the buffer size declaration above: `char md5_string[MD5_BUFSIZE];`
3. Here, use the known buffer size: `snprintf(&(data->md5_string[i * 2]), MD5_BUFSIZE - (i * 2), "%02x", static_cast<unsigned int>(digest[i]));
Alternatively to defining `MD5_BUFSIZE`, `sizeof(data->md5_string)` could be used as well.
##########
tools/jtest/jtest.cc:
##########
@@ -3083,24 +3088,25 @@ build_request(int sock)
if (nheaders > 0) {
char *eh = eheaders;
if (!vary_user_agent) {
- eh += sprintf(eh, "User-Agent: Mozilla/4.04 [en] (X11; I; Linux 2.0.31 i586)\r\n");
+ eh += snprintf(eh, sizeof(eheaders) - (eh - eheaders), "User-Agent: Mozilla/4.04 [en] (X11; I; Linux 2.0.31 i586)\r\n");
nheaders--;
}
if (nheaders > 0) {
- eh += sprintf(eh, "Accept: image/gif, image/x-xbitmap, image/jpeg, image/pjpeg, image/png, */*\r\n");
+ eh += snprintf(eh, sizeof(eheaders) - (eh - eheaders),
+ "Accept: image/gif, image/x-xbitmap, image/jpeg, image/pjpeg, image/png, */*\r\n");
}
while (--nheaders > 0) {
- eh += sprintf(eh, "Extra-Header%d: a lot of junk for header %d\r\n", nheaders, nheaders);
+ eh += snprintf(eh, sizeof(eheaders) - (eh - eheaders), "Extra-Header%d: a lot of junk for header %d\r\n", nheaders, nheaders);
Review Comment:
I don't think this accumulating `eh` index will work out if there is an overflow, right? `snprintf` will return the number of bytes it would have written regardless of the buffer size parameter. Thus, if it only writes 50 bytes because that's the buffer size, but it would have written 105, it will return 105. Then what happens to the subsequent snprintf buffer calculations when the `eh` value is larger than `sizeof(eheaders)`?
##########
plugins/compress/misc.cc:
##########
@@ -168,10 +168,9 @@ init_hidden_header_name()
if (TSMgmtStringGet(var_name, &result) != TS_SUCCESS) {
fatal("failed to get server name");
} else {
- int hidden_header_name_len = strlen("x-accept-encoding-") + strlen(result);
- hidden_header_name = static_cast<char *>(TSmalloc(hidden_header_name_len + 1));
- hidden_header_name[hidden_header_name_len] = 0;
- sprintf(hidden_header_name, "x-accept-encoding-%s", result);
+ int hidden_header_name_len = strlen("x-accept-encoding-") + strlen(result) + 1; // add one for null
+ hidden_header_name = static_cast<char *>(TSmalloc(hidden_header_name_len));
+ snprintf(hidden_header_name, hidden_header_name_len, "x-accept-encoding-%s", result);
Review Comment:
A minor thing, but typically "len" variables refer to the number of characters while `siz` or `size` variables refer to the buffer size. That corresponds to strlen and sizeof. I would say we should either rename the variable to `hidden_header_name_size` and keep your computations (where the variable includes the + 1 for the null terminator), or keep the computations as is and keep the name with `_len`.
##########
plugins/prefetch/plugin.cc:
##########
@@ -584,8 +584,9 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata)
TSHttpHdrReasonSet(bufp, hdrLoc, reason, reasonLen);
PrefetchDebug("set response: %d %.*s '%s'", data->_status, reasonLen, reason, data->_body.c_str());
- char *buf = static_cast<char *>(TSmalloc(data->_body.length() + 1));
- sprintf(buf, "%s", data->_body.c_str());
+ int length = data->_body.length() + 1;
Review Comment:
`bufsize`
##########
proxy/http/remap/unit-tests/plugin_testing_common.cc:
##########
@@ -45,8 +45,9 @@ getTemporaryDir()
fs::path tmpDir = fs::canonical(fs::temp_directory_path(), ec);
tmpDir /= "sandbox_XXXXXX";
- char dirNameTemplate[tmpDir.string().length() + 1];
- sprintf(dirNameTemplate, "%s", tmpDir.c_str());
+ int length = tmpDir.string().length() + 1;
Review Comment:
`bufsize`
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -399,8 +399,9 @@ contHandleAccessControl(const TSCont contp, TSEvent event, void *edata)
/* Don't set any cookie, fail the request here returning appropriate status code and body.*/
TSHttpTxnStatusSet(txnp, config->_invalidOriginResponse);
static const char *body = "Unexpected Response From the Origin Server\n";
- char *buf = static_cast<char *>(TSmalloc(strlen(body) + 1));
- sprintf(buf, "%s", body);
+ int length = strlen(body) + 1;
Review Comment:
length -> bufsize
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1020503048
##########
plugins/experimental/ja3_fingerprint/ja3_fingerprint.cc:
##########
@@ -318,8 +318,10 @@ client_hello_ja3_handler(TSCont contp, TSEvent event, void *edata)
unsigned char digest[MD5_DIGEST_LENGTH];
MD5((unsigned char *)data->ja3_string.c_str(), data->ja3_string.length(), digest);
+ // Validate that the buffer is the same size as we will be writing into (compile time)
+ static_assert((16 * 2 + 1) == sizeof(data->md5_string));
Review Comment:
Good idea
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] moonchen commented on a diff in pull request #9189: Removed most sprintf and ignore sprintf warnings in bind code
Posted by GitBox <gi...@apache.org>.
moonchen commented on code in PR #9189:
URL: https://github.com/apache/trafficserver/pull/9189#discussion_r1020516102
##########
plugins/experimental/access_control/utils.cc:
##########
@@ -124,7 +124,7 @@ urlEncode(const char *in, size_t inLen, char *out, size_t outLen)
*dst++ = '+';
} else {
*dst++ = '%';
- sprintf(dst, "%02x", static_cast<unsigned char>(*src));
Review Comment:
I think this should be `snprintf(dst, out + outLen - dst, "%02x", static_cast<unsigned char>(*src));`.
##########
plugins/prefetch/plugin.cc:
##########
@@ -584,8 +584,9 @@ contHandleFetch(const TSCont contp, TSEvent event, void *edata)
TSHttpHdrReasonSet(bufp, hdrLoc, reason, reasonLen);
PrefetchDebug("set response: %d %.*s '%s'", data->_status, reasonLen, reason, data->_body.c_str());
- char *buf = static_cast<char *>(TSmalloc(data->_body.length() + 1));
- sprintf(buf, "%s", data->_body.c_str());
+ size_t bufsize = data->_body.length() + 1;
+ char *buf = static_cast<char *>(TSmalloc(bufsize));
Review Comment:
I think that `memcpy(buf, data->_body.c_str(), bufsize)` would be faster and simpler here.
##########
proxy/http/remap/unit-tests/plugin_testing_common.cc:
##########
@@ -46,7 +46,7 @@ getTemporaryDir()
tmpDir /= "sandbox_XXXXXX";
char dirNameTemplate[tmpDir.string().length() + 1];
- sprintf(dirNameTemplate, "%s", tmpDir.c_str());
+ snprintf(dirNameTemplate, sizeof(dirNameTemplate), "%s", tmpDir.c_str());
Review Comment:
Likewise, I think this should be `memcpy(dirNameTemplate, tmpDir.c_str(), sizeof(dirNameTemplate));`
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org