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 2021/07/29 00:15:29 UTC
[GitHub] [trafficserver] masaori335 opened a new pull request #8184: Fix a use-after-free reported by clang-analyzer
masaori335 opened a new pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184
Address a use-after-free on top of #8182.
https://ci.trafficserver.apache.org/clang-analyzer/github/8182/2021-07-28-135835-24348-1/report-26932e.html#EndPath
--
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] masaori335 commented on pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#issuecomment-890607670
Please merge without quashing to keep the revert commit from @tomoatan.
--
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] SolidWallOfCode commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r680015094
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
+ free(e->ethread);
Review comment:
Should change to `this->free(e->thread);` to be clear on what's being invoked.
--
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] masaori335 commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r678744723
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
Review comment:
@SolidWallOfCode @bneradt What do you think about release the lock here to make the clang-analyzer happy.
--
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] SolidWallOfCode commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r680063073
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
Review comment:
Looking at the clang report, I don't understand it. Step 17 notes "Memory is released". That's fine - the `UnixNetVC` is destroyed. But what freed memory is used at step 20? That mutex is in the `NetHandler`, not the `UnixNetVC`. None of that is in the `UnixNetVC`. I seriously wonder if `free(e->ethread)` is confusing the analyzer and it thinks the `ethread` has been freed (which is touched from the lock destructor). I think it's well worth a shot to change that to `this->free(e->ethread);` and see that fixes the analyzer issue (without the `lock.release()`).
--
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] SolidWallOfCode commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r680014788
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
Review comment:
I don't see how the early release helps, nor how it would double free. The lock remembers state so it won't try to release the lock again.
--
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] masaori335 commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r678775428
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
Review comment:
Does this make double free?
--
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] zwoop commented on pull request #8184: Revert "Remove UnixNetVConnection::startEvent - not actually called. (#7596)" + clang-analyzer fix
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#issuecomment-892213221
Cherry-picked to v9.0.x branch.
Cherry-picked to v9.1.x branch.
--
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] masaori335 commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r680581781
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
+ free(e->ethread);
Review comment:
It looks like this confused the `clang-analyzer`. Adding `this` fixed the report.
--
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] masaori335 commented on a change in pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#discussion_r680581701
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -990,6 +993,23 @@ UnixNetVConnection::netActivity(EThread *lthread)
net_activity(this, lthread);
}
+int
+UnixNetVConnection::startEvent(int /* event ATS_UNUSED */, Event *e)
+{
+ MUTEX_TRY_LOCK(lock, get_NetHandler(e->ethread)->mutex, e->ethread);
+ if (!lock.is_locked()) {
+ e->schedule_in(HRTIME_MSECONDS(net_retry_delay));
+ return EVENT_CONT;
+ }
+ if (!action_.cancelled) {
+ connectUp(e->ethread, NO_FD);
+ } else {
+ lock.release();
Review comment:
Ah, this change doesn't make sense. I thought the `mutex` is owned by `UnixNetVC` but it's owned by `NetHandler`, as you pointed. I'll try the `this->free(e->ethread)`.
--
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] masaori335 edited a comment on pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 edited a comment on pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#issuecomment-890607670
Please merge without squashing to keep the revert commit from @tomoatan.
--
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] masaori335 commented on pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#issuecomment-888726517
It's a bit weird that we don't see this clang-analyzer report on 8.1.x (#8180). Do we have any diffs?
--
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] masaori335 merged pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 merged pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184
--
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] masaori335 edited a comment on pull request #8184: Fix a use-after-free reported by clang-analyzer
Posted by GitBox <gi...@apache.org>.
masaori335 edited a comment on pull request #8184:
URL: https://github.com/apache/trafficserver/pull/8184#issuecomment-890607670
~~Please merge without squashing to keep the revert commit from @tomoatan.~~ We have the "Squash & merge" option only. I'll modify the commit message.
--
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