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/06/14 22:19:07 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request, #8913: Change TSMutexCreate/Destroy to use mutex reference counts.
ywkaras opened a new pull request, #8913:
URL: https://github.com/apache/trafficserver/pull/8913
Also has some very minor cleanup of Ptr.h.
Core events refer to the mutex with Ptr<ProxyMutex>. So, when a plugin continuation
is done executing, and free_event() is called, and the continuation has a mutex, the
mutex's reference count may become zero, and the mutex will be deleted. Yahoo prod
is seeing crashes that seem to be due to this problem.
I think we don't see more crashes from this because plugin continuations that are
directly scheduled typically reschedule themselves before exiting. Thus, they create
a new event, referencing the mutex, before the current one is freed.
--
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] shukitchan commented on pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
Posted by GitBox <gi...@apache.org>.
shukitchan commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1201923562
> I think these lines of code may potentially cause this crash: https://git.ouryahoo.com/mirror-github/SolidWallOfCode--txn_box/blob/0.4.5/plugin/src/ts_util.cc#L1030 https://git.ouryahoo.com/mirror-github/SolidWallOfCode--txn_box/blob/0.4.5/plugin/src/ts_util.cc#L1003
>
> The continuation has the only Ptr reference to the mutex. So, when the continuation is destroyed, the reference is cleared, causing the mutex to be destroyed, while it's still locked.
I think these links are internal to yahoo only.
--
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] github-actions[bot] commented on pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1451166542
This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1201853977
Example stack dump from crash. The line numbers will be off, it's Yahoo internal TS.
```
[ 00 ] libc-2.17.so raise ( raise.c:55 )
[ 01 ] libc-2.17.so abort ( abort.c:90 )
[ 02 ] libtscore.so.9.1.10 ink_abort ( ink_error.cc:99 )
[ 03 ] libtscore.so.9.1.10 ink_mutex_destroy ( ink_mutex.cc:72 )
[ 04 ] traffic_server free ( I_Lock.h:595 )
[ 05 ] traffic_server operator= ( Ptr.h:241 )
[ 06 ] traffic_server free_event ( P_UnixEThread.h:251 )
[ 07 ] traffic_server EThread::process_queue(Queue<Event, Event::Link_link>*, int*, int*) ( UnixEThread.cc:196 )
[ 08 ] traffic_server EThread::execute_regular() ( UnixEThread.cc:259 )
[ 09 ] traffic_server execute ( UnixEThread.cc:364 )
[ 10 ] traffic_server EThread::execute() ( UnixEThread.cc:342 )
[ 11 ] traffic_server spawn_thread_internal ( Thread.cc:91 )
[ 12 ] libpthread-2.17.so start_thread ( pthread_create.c:307 )
```
--
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 closed pull request #8913: Change TSMutexCreate/Destroy to use mutex reference counts.
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #8913: Change TSMutexCreate/Destroy to use mutex reference counts.
URL: https://github.com/apache/trafficserver/pull/8913
--
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] github-actions[bot] closed pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
URL: https://github.com/apache/trafficserver/pull/8913
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1170623162
[approve ci autest]
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1201859689
I think these lines of code may potentially cause this crash:
https://git.ouryahoo.com/mirror-github/SolidWallOfCode--txn_box/blob/0.4.5/plugin/src/ts_util.cc#L1030
https://git.ouryahoo.com/mirror-github/SolidWallOfCode--txn_box/blob/0.4.5/plugin/src/ts_util.cc#L1003
The continuation has the only Ptr reference to the mutex. So, when the continuation is destroyed, the reference is cleared, causing the mutex to be destroyed, while it's still locked.
--
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 #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1177889571
I think this looks good, has it been tested in prod?
--
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 pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1238356442
[approve ci autest]
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1201487436
This fixed a crash Yahoo was seeing in txn_box. txn_box was destroying a continuation with a mutex in the continuation function (so the mutex was locked). Continuations have a (Ptr) reference to their mutex, and the continuation's reference was the only one to its mutex. So the mutex got deleted while it was locked.
@SolidWallOfCode says he will change this code to get the mutex from the continuation, and unlock it, before destroying the continuation. So it boils down to whether we want to idiot proof the mutex destroy, so it will unlock if necessary before destruction.
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1201473980
> I think this looks good, has it been tested in prod?
Yes, now it has.
--
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 #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#discussion_r910493628
##########
iocore/eventsystem/UnixEThread.cc:
##########
@@ -142,6 +142,7 @@ EThread::process_event(Event *e, int calling_code)
EventQueueExternal.enqueue_local(e);
} else {
if (e->cancelled) {
+ MUTEX_RELEASE(lock);
Review Comment:
@SolidWallOfCode Yahoo prod is seeing some crashes in the free_event() function. Looking at a core file, they seem to be caused by the continuation functions that destroy their own continuation functions in ts_utils.cc in txn_box. I think we need these two unlocks before the free_event() calls to prevent trafficserver from crashing due to destruction of a locked 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 pull request #8913: Change TSMutexCreate/Destroy to use mutex reference counts.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1170186043
This isn't necessary because INKContInternal contains a Continuation which points to the mutex with Ptr<ProxyMutex>.
--
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 pull request #8913: In event processing, assert that mutex is unlocked before freeing mutex.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1202709511
> I think these links are internal to yahoo only.
Oops, fixed.
--
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 pull request #8913: Change TSMutexCreate/Destroy to use mutex reference counts.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1170225136
On second though, this is still desirable. TSMutexDestroy() could cause a use after free otherwise. TSContCreate() could cause a use after free if the mutex was previously used by a destroyed continuation.
--
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 pull request #8913: In event processing, unlock event mutex before freeing event.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on PR #8913:
URL: https://github.com/apache/trafficserver/pull/8913#issuecomment-1178249891
There's problems with it, I'm working on a fix.
--
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