Mercurial > hg > release > heapstats-2.0
changeset 190:61d4732ddf95
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:07:20 +0900 |
| parents | 029ce36d58a7 |
| children | 1eda4c84297f |
| files | ChangeLog agent/src/heapstats-engines/arch/arm/util.inline.hpp agent/src/heapstats-engines/arch/x86/util.inline.hpp agent/src/heapstats-engines/classContainer.cpp agent/src/heapstats-engines/snapShotContainer.cpp agent/src/heapstats-engines/snapShotContainer.hpp agent/src/heapstats-engines/snapShotMain.cpp agent/src/heapstats-engines/util.hpp |
| diffstat | 8 files changed, 186 insertions(+), 29 deletions(-) [+] |
line wrap: on
line diff
--- a/ChangeLog Mon Jan 16 23:55:33 2017 +0900 +++ b/ChangeLog Tue Jan 17 12:07:20 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 3285: JVM aborted on assert code at MonitorContended JVMTI event
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/agent/src/heapstats-engines/arch/arm/util.inline.hpp Tue Jan 17 12:07:20 2017 +0900 @@ -0,0 +1,42 @@ +/*! + * \file util.inline.hpp + * \brief Optimized utility functions for ARM processor. + * Copyright (C) 2017 Yasumasa Suenaga + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + */ + +#ifndef ARM_UTIL_H +#define ARM_UTIL_H + +inline void atomic_inc(int *target, int value) { + asm volatile("1:" + " ldrex %%r0, [%0];" + " add %%r0, %%r0, %1;" + " strex %%r1, %%r0, [%0];" + " tst %%r1, %%r1;" + " bne 1b;" + : : "r"(target), "r"(value) : "memory", "cc", "%r0", "%r1"); +} + +inline int atomic_get(int *target) { + register int ret; + asm volatile("ldrex %0, [%1]" : "=r"(ret) : "r"(target) : ); + + return ret; +} + +#endif // ARM_UTIL_H
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/agent/src/heapstats-engines/arch/x86/util.inline.hpp Tue Jan 17 12:07:20 2017 +0900 @@ -0,0 +1,46 @@ +/*! + * \file util.inline.hpp + * \brief Optimized utility functions for x86 processor. + * Copyright (C) 2017 Yasumasa Suenaga + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + */ + +#ifndef X86_UTIL_H +#define X86_UTIL_H + +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 // X86_UTIL_H
--- a/agent/src/heapstats-engines/classContainer.cpp Mon Jan 16 23:55:33 2017 +0900 +++ b/agent/src/heapstats-engines/classContainer.cpp Tue Jan 17 12:07:20 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 @@ -976,7 +976,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, * iterator's relationship will be broken.
--- a/agent/src/heapstats-engines/snapShotContainer.cpp Mon Jan 16 23:55:33 2017 +0900 +++ b/agent/src/heapstats-engines/snapShotContainer.cpp Tue Jan 17 12:07:20 2017 +0900 @@ -205,6 +205,7 @@ counter = counter->next; /* Deallocate TChildClassCounter. */ + atomic_inc(&aCounter->objData->numRefsFromChildren, -1); free(aCounter->counter); free(aCounter); } @@ -299,6 +300,7 @@ return NULL; } + atomic_inc(&objData->numRefsFromChildren, 1); this->clearObjectCounter(newCounter->counter); newCounter->objData = objData; @@ -473,10 +475,13 @@ TObjectData *objData = counter->objData; /* - * If the class of objData is already unloaded, we should remove - * reference to it from child object data. + * If the class of objData is already unloaded, we should 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) { @@ -489,6 +494,25 @@ free(counter->counter); free(counter); + /* 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. */
--- a/agent/src/heapstats-engines/snapShotContainer.hpp Mon Jan 16 23:55:33 2017 +0900 +++ b/agent/src/heapstats-engines/snapShotContainer.hpp Tue Jan 17 12:07:20 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file snapshotContainer.hpp * \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 @@ -67,18 +67,21 @@ /*! * \brief This structure stored class information. */ +#pragma pack(push, 4) typedef struct { - jlong tag; /*!< Class tag. */ - jlong classNameLen; /*!< Class name. */ - char *className; /*!< Class name length. */ - void *klassOop; /*!< Java inner class object. */ - jlong oldTotalSize; /*!< Class old total use size. */ - TOopType oopType; /*!< Type of class. */ - jlong clsLoaderId; /*!< Class loader instance id. */ - jlong clsLoaderTag; /*!< Class loader class tag. */ - bool isRemoved; /*!< Class is already unloaded. */ - jlong instanceSize; /*!< Class size if this class is instanceKlass. */ + jlong tag; /*!< Class tag. */ + jlong classNameLen; /*!< Class name. */ + char *className; /*!< Class name length. */ + void *klassOop; /*!< Java inner class object. */ + jlong oldTotalSize; /*!< Class old total use size. */ + TOopType oopType; /*!< Type of class. */ + jlong clsLoaderId; /*!< Class loader instance id. */ + 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; +#pragma pack(pop) /*! * \brief This structure stored child class size information. @@ -260,6 +263,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. @@ -272,21 +307,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/heapstats-engines/snapShotMain.cpp Mon Jan 16 23:55:33 2017 +0900 +++ b/agent/src/heapstats-engines/snapShotMain.cpp Tue Jan 17 12:07:20 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 @@ -378,7 +378,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/heapstats-engines/util.hpp Mon Jan 16 23:55:33 2017 +0900 +++ b/agent/src/heapstats-engines/util.hpp Tue Jan 17 12:07:20 2017 +0900 @@ -1,7 +1,7 @@ /*! * \file util.hpp * \brief This file is utilities. - * 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 @@ -364,6 +364,14 @@ }; /* CPU Specific utilities. */ +#if PROCESSOR_ARCH == X86 +#include "arch/x86/util.inline.hpp" +#elif PROCESSOR_ARCH == ARM +#include "arch/arm/util.inline.hpp" +#else +#error "Unknown CPU architecture." +#endif + #ifdef AVX #include "arch/x86/avx/util.hpp" #elif defined(SSE2) || defined(SSE3) || defined(SSE4)