From 6e3b1703937f8e28df4f69785f1e2d335c0e68d8 Mon Sep 17 00:00:00 2001 From: Ilya Mizus Date: Wed, 9 Apr 2014 10:16:53 +0400 Subject: [PATCH 1/4] Trying to solve the properties memory problem --- src/avian/machine.h | 2 +- src/jnienv.cpp | 4 ++-- src/machine.cpp | 13 ++++++++++++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/avian/machine.h b/src/avian/machine.h index ae8fe8e749..213e0bbe14 100644 --- a/src/avian/machine.h +++ b/src/avian/machine.h @@ -1263,7 +1263,7 @@ class Machine { Thread* exclusive; Thread* finalizeThread; Reference* jniReferences; - const char** properties; + char** properties; unsigned propertyCount; const char** arguments; unsigned argumentCount; diff --git a/src/jnienv.cpp b/src/jnienv.cpp index 6872d775f1..0debe43d1f 100644 --- a/src/jnienv.cpp +++ b/src/jnienv.cpp @@ -3940,10 +3940,10 @@ JNI_CreateJavaVM(Machine** m, Thread** t, void* args) } unsigned cpl = strlen(classpath); - RUNTIME_ARRAY(char, classpathProperty, cpl + sizeof(CLASSPATH_PROPERTY) + 1); + RUNTIME_ARRAY(char, classpathProperty, cpl + strlen(CLASSPATH_PROPERTY) + 2); if (addClasspathProperty) { char* p = RUNTIME_ARRAY_BODY(classpathProperty); - local::append(&p, CLASSPATH_PROPERTY, sizeof(CLASSPATH_PROPERTY), '='); + local::append(&p, CLASSPATH_PROPERTY, strlen(CLASSPATH_PROPERTY), '='); local::append(&p, classpath, cpl, 0); *(propertyPointer++) = RUNTIME_ARRAY_BODY(classpathProperty); } diff --git a/src/machine.cpp b/src/machine.cpp index 74719cdbc2..f93dc083ab 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -3119,7 +3119,6 @@ Machine::Machine(System* system, Heap* heap, Finder* bootFinder, exclusive(0), finalizeThread(0), jniReferences(0), - properties(properties), propertyCount(propertyCount), arguments(arguments), argumentCount(argumentCount), @@ -3164,6 +3163,14 @@ Machine::Machine(System* system, Heap* heap, Finder* bootFinder, if (codeLibraryName && (codeLibraryNameEnd = strchr(codeLibraryName, system->pathSeparator()))) *codeLibraryNameEnd = 0; + // Copying the properties memory (to avoid memory crashes) + this->properties = (char**)heap->allocate(sizeof(char*) * propertyCount); + for (unsigned int i = 0; i < propertyCount; i++) + { + this->properties[i] = (char*)heap->allocate(sizeof(char) * (strlen(properties[i]) + 1)); + strcpy(this->properties[i], properties[i]); + } + if (not system->success(system->make(&localThread)) or not system->success(system->make(&stateLock)) or not system->success(system->make(&heapLock)) or @@ -3222,6 +3229,10 @@ Machine::dispose() heap->free(arguments, sizeof(const char*) * argumentCount); + for (unsigned int i = 0; i < propertyCount; i++) + { + heap->free(properties[i], sizeof(char) * (strlen(properties[i]) + 1)); + } heap->free(properties, sizeof(const char*) * propertyCount); static_cast(heapClient)->dispose(); From 5828ef1d46bcff32c7e861b863022aae39cb7862 Mon Sep 17 00:00:00 2001 From: Vasily Litvinov Date: Wed, 9 Apr 2014 16:02:48 +0400 Subject: [PATCH 2/4] Fixing property copying --- src/machine.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/machine.cpp b/src/machine.cpp index f93dc083ab..ad1fb928b4 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -3155,14 +3155,6 @@ Machine::Machine(System* system, Heap* heap, Finder* bootFinder, populateJNITables(&javaVMVTable, &jniEnvVTable); - const char* bootstrapProperty = findProperty(this, BOOTSTRAP_PROPERTY); - const char* bootstrapPropertyDup = bootstrapProperty ? strdup(bootstrapProperty) : 0; - const char* bootstrapPropertyEnd = bootstrapPropertyDup + (bootstrapPropertyDup ? strlen(bootstrapPropertyDup) : 0); - char* codeLibraryName = (char*)bootstrapPropertyDup; - char* codeLibraryNameEnd = 0; - if (codeLibraryName && (codeLibraryNameEnd = strchr(codeLibraryName, system->pathSeparator()))) - *codeLibraryNameEnd = 0; - // Copying the properties memory (to avoid memory crashes) this->properties = (char**)heap->allocate(sizeof(char*) * propertyCount); for (unsigned int i = 0; i < propertyCount; i++) @@ -3171,6 +3163,14 @@ Machine::Machine(System* system, Heap* heap, Finder* bootFinder, strcpy(this->properties[i], properties[i]); } + const char* bootstrapProperty = findProperty(this, BOOTSTRAP_PROPERTY); + const char* bootstrapPropertyDup = bootstrapProperty ? strdup(bootstrapProperty) : 0; + const char* bootstrapPropertyEnd = bootstrapPropertyDup + (bootstrapPropertyDup ? strlen(bootstrapPropertyDup) : 0); + char* codeLibraryName = (char*)bootstrapPropertyDup; + char* codeLibraryNameEnd = 0; + if (codeLibraryName && (codeLibraryNameEnd = strchr(codeLibraryName, system->pathSeparator()))) + *codeLibraryNameEnd = 0; + if (not system->success(system->make(&localThread)) or not system->success(system->make(&stateLock)) or not system->success(system->make(&heapLock)) or From 647e22bf81a83ba2186ab8fa5fab39b5641c49bc Mon Sep 17 00:00:00 2001 From: Vasily Litvinov Date: Wed, 9 Apr 2014 19:36:34 +0400 Subject: [PATCH 3/4] Fixed memory leak (which triggered asserts in tests) --- src/jnienv.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/jnienv.cpp b/src/jnienv.cpp index 0debe43d1f..107c4a697b 100644 --- a/src/jnienv.cpp +++ b/src/jnienv.cpp @@ -3958,6 +3958,8 @@ JNI_CreateJavaVM(Machine** m, Thread** t, void* args) (s, h, bf, af, p, c, properties, propertyCount, arguments, a->nOptions, stackLimit); + h->free(properties, sizeof(const char*) * propertyCount); + *t = p->makeThread(*m, 0, 0); enter(*t, Thread::ActiveState); From 74209edb7d00ca953b1553e37bb3a3933087e8e0 Mon Sep 17 00:00:00 2001 From: Vasily Litvinov Date: Thu, 10 Apr 2014 01:02:11 +0400 Subject: [PATCH 4/4] Replacing strcpy with memcpy - should be slightly faster because we're forced to know strlen, so no need in byte-by-byte copying --- src/machine.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/machine.cpp b/src/machine.cpp index ad1fb928b4..29f1a8a270 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -3159,8 +3159,9 @@ Machine::Machine(System* system, Heap* heap, Finder* bootFinder, this->properties = (char**)heap->allocate(sizeof(char*) * propertyCount); for (unsigned int i = 0; i < propertyCount; i++) { - this->properties[i] = (char*)heap->allocate(sizeof(char) * (strlen(properties[i]) + 1)); - strcpy(this->properties[i], properties[i]); + size_t length = strlen(properties[i]) + 1; // +1 for null-terminating char + this->properties[i] = (char*)heap->allocate(sizeof(char) * length); + memcpy(this->properties[i], properties[i], length); } const char* bootstrapProperty = findProperty(this, BOOTSTRAP_PROPERTY);