You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "maskit (via GitHub)" <gi...@apache.org> on 2023/05/02 20:52:56 UTC
[GitHub] [trafficserver] maskit opened a new pull request, #9672: Fixes crashes around OCSP with FetchSM
maskit opened a new pull request, #9672:
URL: https://github.com/apache/trafficserver/pull/9672
We saw two kind of crashes while testing #9591 and #9624, and the both were triggered by OCSP request timeout. In a nutshell, the crashes are due to accessing unavailable continuations. Continuations become unavailable when OCSP requests timeout, but the status were not properly updated/checked.
--
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] ywkaras commented on a diff in pull request #9672: Fixes crashes around OCSP with FetchSM
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9672:
URL: https://github.com/apache/trafficserver/pull/9672#discussion_r1188090274
##########
iocore/net/OCSPStapling.cc:
##########
@@ -1016,13 +1028,21 @@ query_responder(const char *uri, const char *user_agent, TS_OCSP_REQUEST *req, i
}
// Send request
- eventProcessor.schedule_imm(&httpreq, ET_NET);
+ Event *e = eventProcessor.schedule_imm(&httpreq, ET_NET);
// Wait until the request completes
do {
ink_hrtime_sleep(HRTIME_MSECONDS(1));
} while (!httpreq.is_done() && (Thread::get_hrtime() < end));
+ if (!httpreq.is_done()) {
+ Error("OCSP request was timed out; uri=%s", uri);
+ if (!httpreq.is_initiated()) {
+ Debug("ssl_ocsp", "Request is not initiated yet. Cancelling the event.");
+ e->cancel(&httpreq);
+ }
+ }
Review Comment:
Add a `}' to close the block and release the mutex.
--
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] ywkaras commented on a diff in pull request #9672: Fixes crashes around OCSP with FetchSM
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9672:
URL: https://github.com/apache/trafficserver/pull/9672#discussion_r1188076097
##########
iocore/net/OCSPStapling.cc:
##########
@@ -355,31 +355,43 @@ class HTTPRequest : public Continuation
fetch()
{
SCOPED_MUTEX_LOCK(lock, mutex, this_ethread());
+ this->_result = 0;
Review Comment:
In `event_handler()`, won't `req` always be the same as `this`? What's the point of it then?
--
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] maskit merged pull request #9672: Fixes crashes around OCSP with FetchSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9672:
URL: https://github.com/apache/trafficserver/pull/9672
--
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] maskit commented on a diff in pull request #9672: Fixes crashes around OCSP with FetchSM
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9672:
URL: https://github.com/apache/trafficserver/pull/9672#discussion_r1190235795
##########
iocore/net/OCSPStapling.cc:
##########
@@ -355,31 +355,43 @@ class HTTPRequest : public Continuation
fetch()
{
SCOPED_MUTEX_LOCK(lock, mutex, this_ethread());
+ this->_result = 0;
Review Comment:
I moved the functions private and removed unnecessary calls for ext_set/get_user_data.
> So, the continuation mutex will already be locked when they are called.
This may be true, but I kept `SCOPED_MUTEX_LOCK`, which should be almost no-op if the lock is acquired. Because `e->mutex` and `e->continuation->mutex` can be different under some conditions and `e->continuation->mutex` is not locked in that case. It may not be the case for this, but I'd rather explicitly acquire the lock.
##########
iocore/net/OCSPStapling.cc:
##########
@@ -355,31 +355,43 @@ class HTTPRequest : public Continuation
fetch()
{
SCOPED_MUTEX_LOCK(lock, mutex, this_ethread());
+ this->_result = 0;
Review Comment:
I made the functions private and removed unnecessary calls for ext_set/get_user_data.
> So, the continuation mutex will already be locked when they are called.
This may be true, but I kept `SCOPED_MUTEX_LOCK`, which should be almost no-op if the lock is acquired. Because `e->mutex` and `e->continuation->mutex` can be different under some conditions and `e->continuation->mutex` is not locked in that case. It may not be the case for this, but I'd rather explicitly acquire the lock.
--
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] ywkaras commented on a diff in pull request #9672: Fixes crashes around OCSP with FetchSM
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9672:
URL: https://github.com/apache/trafficserver/pull/9672#discussion_r1188008934
##########
iocore/net/OCSPStapling.cc:
##########
@@ -355,31 +355,43 @@ class HTTPRequest : public Continuation
fetch()
{
SCOPED_MUTEX_LOCK(lock, mutex, this_ethread());
+ this->_result = 0;
Review Comment:
Looking at this:
```
wkaras ~/REPOS/TS
O$ grep -e '\Wfetch(' -e '\Wset_done(' -e '\Wset_error(' $(findsrc)
./include/tscore/ArgParser.h: void set_error(std::string e);
./iocore/net/OCSPStapling.cc: this->fetch();
./iocore/net/OCSPStapling.cc: req->set_done();
./iocore/net/OCSPStapling.cc: req->set_error();
./iocore/net/OCSPStapling.cc: fetch()
./iocore/net/OCSPStapling.cc: set_done()
./iocore/net/OCSPStapling.cc: set_error()
./plugins/experimental/inliner/cache.h: fetch(const std::string &k, A &&...a)
./plugins/experimental/slice/prefetch.cc: if (bg->fetch(data)) {
./plugins/experimental/slice/prefetch.cc:BgBlockFetch::fetch(Data *const data)
./plugins/experimental/slice/prefetch.h: bool fetch(Data *const data);
./plugins/lua/ts_lua_fetch.c: /* ts.fetch() */
./src/tscore/ArgParser.cc:ArgParser::set_error(std::string e)
./src/tscore/unit_tests/test_ArgParser.cc: parser2.set_error("error");
wkaras ~/REPOS/TS
O$
```
it's clear that `fetch()`, `set_done()` and `set_error()` are only called by `event_handler()`. So, the continuation mutex will already be locked when they are called. It also means these member functions should be private. I suggest you make them private as a part of this PR.
--
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