Mercurial > hg > release > heapstats-1.1
changeset 60:eafa619db958
Bug 3284: HeapStats Agent might crash when object children are scanned
Reviewed-by: ykubota
https://github.com/HeapStats/heapstats/pull/74
| author | Yasumasa Suenaga <yasuenag@gmail.com> |
|---|---|
| date | Tue, 17 Jan 2017 12:08:51 +0900 |
| parents | d766054a28ee |
| children | 9722a1848310 |
| files | agent/ChangeLog agent/src/classContainer.cpp agent/src/snapShotContainer.cpp agent/src/snapShotContainer.hpp agent/src/snapShotMain.cpp agent/src/util.hpp |
| diffstat | 6 files changed, 98 insertions(+), 20 deletions(-) [+] |
line wrap: on
line diff
--- a/agent/ChangeLog Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/ChangeLog Tue Jan 17 12:08:51 2017 +0900 @@ -1,3 +1,7 @@ +2017-01-17 Yasumasa Suenaga <yasuenag@gmail.com> + + * Bug 3284: HeapStats Agent might crash when object children are scanned + 2017-01-16 Yasumasa Suenaga <yasuenag@gmail.com> * Bug 3104: SEGV in TSnapShotContainer::mergeChildren with Oracle JDK8u92
--- a/agent/src/classContainer.cpp Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/src/classContainer.cpp Tue Jan 17 12:08:51 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file classContainer.cpp * \brief This file is used to add up using size every class. - * Copyright (C) 2011-2015 Nippon Telegraph and Telephone Corporation + * Copyright (C) 2011-2017 Nippon Telegraph and Telephone Corporation * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -1035,8 +1035,8 @@ TObjectData *objData = (*cur).second; /* If class is prepared remove from class container. */ - if (unlikely(objData->oldTotalSize == 0 - && objData->isRemoved)) { + if (unlikely(objData->isRemoved && + (atomic_get(&objData->numRefsFromChildren) == 0))) { /* * If we do removing map item here,
--- a/agent/src/snapShotContainer.cpp Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/src/snapShotContainer.cpp Tue Jan 17 12:08:51 2017 +0900 @@ -222,6 +222,7 @@ counter = counter->next; /* Deallocate TChildClassCounter. */ + atomic_inc(&aCounter->objData->numRefsFromChildren, -1); free(aCounter->counter); free(aCounter); } @@ -349,6 +350,7 @@ : : "r" (newCounter->counter) : "cc", "memory", "%xmm0"); } + atomic_inc(&objData->numRefsFromChildren, 1); newCounter->objData = objData; /* Chain children list. */ @@ -599,9 +601,12 @@ /* * If the class of objData is already unloaded, we should - * remove reference to it from child object data. + * free and shold decrement reference count. + * TChildClassCounter reference count will be checked at + * TClassContainer::commitClassChange(). */ if (objData->isRemoved) { + atomic_inc(&objData->numRefsFromChildren, -1); TChildClassCounter *nextCounter = counter->next; if (prevCounter == NULL) { @@ -614,7 +619,26 @@ free(counter->counter); free(counter); - counter = nextCounter; + /* Deallocate TChildClassCounter in parent container. */ + TChildClassCounter *childClsData, *parentPrevData, + *parentMorePrevData; + this->findChildCounters(clsCounter, objData->klassOop, + &childClsData, &parentPrevData, + &parentMorePrevData); + if (likely(childClsData != NULL)) { + atomic_inc(&objData->numRefsFromChildren, -1); + + if (parentPrevData == NULL) { + clsCounter->child = childClsData->next; + } else { + parentPrevData->next = childClsData->next; + } + + free(childClsData->counter); + free(childClsData); + } + + counter = nextCounter; } else { /* Search child class. */ TChildClassCounter *childClsData =
--- a/agent/src/snapShotContainer.hpp Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/src/snapShotContainer.hpp Tue Jan 17 12:08:51 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file snapshotContainer.hpp * \brief This file is used to add up using size every class. - * Copyright (C) 2011-2014 Nippon Telegraph and Telephone Corporation + * Copyright (C) 2011-2017 Nippon Telegraph and Telephone Corporation * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -86,6 +86,7 @@ jlong clsLoaderTag; /*!< Class loader class tag. */ bool isRemoved; /*!< Class is already unloaded. */ jlong instanceSize; /*!< Class size if this class is instanceKlass. */ + int numRefsFromChildren; /*!< Number of references from TChildClassCounter. */ } TObjectData; /*! @@ -321,6 +322,38 @@ } /*! + * \brief Find child class and its prevous data. + * \param clsCounter [in] Parent class counter object. + * \param klassOop [in] Child class key object. + * \param counter [out] Child data + * \param prevCounter [out] previous counter of `counter` + * \param morePrevCounter [out] previous counter of `prevCounter` + */ + inline void findChildCounters(TClassCounter *clsCounter, void *klassOop, + TChildClassCounter **counter, + TChildClassCounter** prevCounter, + TChildClassCounter **morePrevCounter) { + *prevCounter = NULL; + *morePrevCounter = NULL; + *counter = clsCounter->child; + + if (*counter == NULL) { + return; + } + + /* Search children class list. */ + while ((*counter)->objData->klassOop != klassOop) { + *morePrevCounter = *prevCounter; + *prevCounter = *counter; + *counter = (*counter)->next; + + if (*counter == NULL) { + return; + } + } + }; + + /*! * \brief Find child class data. * \param clsCounter [in] Parent class counter object. * \param klassOop [in] Child class key object. @@ -333,22 +366,13 @@ TChildClassCounter *morePrevCounter = NULL; TChildClassCounter *counter = clsCounter->child; + this->findChildCounters(clsCounter, klassOop, + &counter, &prevCounter, &morePrevCounter); + if(counter == NULL){ return NULL; } - /* Search children class list. */ - while(counter->objData->klassOop != klassOop){ - morePrevCounter = prevCounter; - prevCounter = counter; - counter = counter->next; - - if(counter == NULL){ - return NULL; - } - - } - /* LFU (Least Frequently Used). */ if (counter != NULL) { counter->callCount++;
--- a/agent/src/snapShotMain.cpp Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/src/snapShotMain.cpp Tue Jan 17 12:08:51 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file snapShotMain.cpp * \brief This file is used to take snapshot. - * Copyright (C) 2011-2015 Nippon Telegraph and Telephone Corporation + * Copyright (C) 2011-2017 Nippon Telegraph and Telephone Corporation * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -405,7 +405,12 @@ TObjectData *clsData = getObjectDataFromKlassOop( localClsContainer, klassOop); /* Push new child loaded class. */ - clsCounter = localSnapshot->pushNewChildClass(parentCounter, clsData); + if (!clsData->isRemoved) { + clsCounter = localSnapshot->pushNewChildClass(parentCounter, clsData); + } else { + /* We should return if clsData has already been removed (unloaded). */ + return; + } } if(unlikely(clsCounter == NULL)){
--- a/agent/src/util.hpp Mon Jan 16 23:56:38 2017 +0900 +++ b/agent/src/util.hpp Tue Jan 17 12:08:51 2017 +0900 @@ -588,4 +588,25 @@ }; }; +inline void atomic_inc(int *target, int value) { + asm volatile("lock addl %1, (%0)" : + : "r"(target), "r"(value) + : "memory", "cc"); +} + +/* + * We can use MOV operation at this point. + * LOCK'ed store operation will be occurred before load (MOV) operation. + * + * Intel (R) 64 and IA-32 Architectures Software Developer’s Manual + * Volume 3A: System Programming Guide, Part 1 + * 8.2.3.8 Locked Instructions Have a Total Order + */ +inline int atomic_get(int *target) { + register int ret; + asm volatile("movl (%1), %0" : "=r"(ret) : "r"(target) : ); + + return ret; +} + #endif