You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "serrislew (via GitHub)" <gi...@apache.org> on 2023/09/07 18:52:26 UTC
[GitHub] [trafficserver] serrislew opened a new pull request, #10384: Coverity 1518612: Remove dead code
serrislew opened a new pull request, #10384:
URL: https://github.com/apache/trafficserver/pull/10384
Fixes: #10332
`out_end` cannot be nullptr, remove conditional
- `*out_len` > 0 is always true
--
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] serrislew merged pull request #10384: Coverity 1518612: Remove dead code
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew merged PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319135357
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
OK, so this isn't an issue with your patch, but I'm concerned this code has a one byte buffer overrun potential:
1. The caller passes in a pointer to a buffer that will be filled called `out` and a length for the allocated space called `out_len`.
2. Then we set `out_end` accordingly: `out_end = out + *out_len;`
3. OK, so `out_end` points to the end of the allocated buffer.
4. Then we increment one past the end of that buffer: `out_end++;`
5. And then write into the `out` buffer pointed to by `w`.
The fact is, we are probably always well under `out_len`, so this doesn't cause problems. But isn’t that a one byte buffer overrun waiting to happen?
In other words, the caller said "Don't write more than `out_len` bytes to `out`." Then we say, "OK, I'll write at most `out_len + 1` bytes to `out`.
Let me know whether I'm missing something.
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319135357
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
OK, so this isn't an issue with your patch, but I'm concerned this code has a one byte buffer overrun potential:
1. The caller passes in a pointer to a buffer that will be filled called `out` and a length for the allocated space called `out_len`.
2. Then we set `out_end` accordingly: `out_end = out + *out_len;`
3. OK, so out_end points to the end of the allocated buffer.
4. Then we increment one past the end of that buffer: `out_end++;`
5. And then write into the `out` buffer pointed to by `w`.
The fact is, we are probably always well under `out_len`, so this doesn't cause problems. But isn’t that a one byte buffer overrun waiting to happen?
In other words, the caller said "Don't write more than `out_len` bytes to `out`." Then we say, "OK, I'll write at most `out_len + 1` bytes to `out`.
Let me know whether I'm missing something.
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
OK, so this isn't an issue with your patch, but I'm concerned this code has a one byte buffer overrun potential:
1. The caller passes in a pointer to a buffer that will be filled called `out` and a length for the allocated space called `out_len`.
2. Then we set `out_end` accordingly: `out_end = out + *out_len;`
3. OK, so out_end points to the end of the allocated buffer.
4. Then we increment one past the end of that buffer: `out_end++;`
5. And then write into the `out` buffer pointed to by `w`.
The fact is, we are probably always well under `out_len`, so this doesn't cause problems. But isn’t that a one byte buffer overrun waiting to happen?
In other words, the caller said "Don't write more than `out_len` bytes to `out`." Then we say, "OK, I'll write at most `out_len + 1` bytes to `out`.
Let me know whether I'm missing something.
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319089441
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
A simple enough change. But for my benefit, can you explain why out_end is always non-zero? It wasn't obvious to me reading the function that it would be. At 211 it's set to:
```cpp
out_end = out + *out_len;
```
I think char is signed with some compilers, so one or both of those could be negative, which complicates things.
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319142266
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
@serrislew pointed out that earlier, in line 202, `out_len` was decremented, so this increment for `\0` should be safe per the incoming buffer.
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319152717
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
I think, in that case, there will be no space for the `out_end++`, right?
--
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] serrislew commented on a diff in pull request #10384: Coverity 1518612: Remove dead code
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319109923
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
maybe always was misleading but `get_stripped` only gets called here [`CookieJar::verify_value`] (https://github.pie.apple.com/ats/trafficserver/blob/master/plugins/experimental/cookie_remap/cookiejar.cc#L178) with the above code setting `out_len` > 0
--
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 #10384: Coverity 1518612: Remove dead code
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #10384:
URL: https://github.com/apache/trafficserver/pull/10384#discussion_r1319152284
##########
plugins/experimental/cookie_remap/strip.cc:
##########
@@ -228,7 +228,7 @@ get_stripped(const char *in, ssize_t in_len, char *out, int *out_len, unsigned i
retval = stripped_core(r, in_end, &w, out_end, flags);
/* null terminate */
- out_end += out_end ? 1 : 0; /* undo decrement at start */
+ out_end++; /* undo decrement at start */
Review Comment:
Should we add a parameter check at line 192 that if `out` is non-nullptr, then `*out_len > 0` ?
--
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