Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid stringop-overflow errors #10293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid stringop-overflow errors #10293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 17 additions & 23 deletions runtime/rastrace/module.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,26 @@
<?xml version="1.0" encoding="UTF-8"?>

<!--
Copyright (c) 2006, 2019 IBM Corp. and others
Copyright (c) 2006, 2020 IBM Corp. and others

This program and the accompanying materials are made available under
the terms of the Eclipse Public License 2.0 which accompanies this
distribution and is available at https://www.eclipse.org/legal/epl-2.0/
or the Apache License, Version 2.0 which accompanies this distribution and
is available at https://www.apache.org/licenses/LICENSE-2.0.
This program and the accompanying materials are made available under
the terms of the Eclipse Public License 2.0 which accompanies this
distribution and is available at https://www.eclipse.org/legal/epl-2.0/
or the Apache License, Version 2.0 which accompanies this distribution and
is available at https://www.apache.org/licenses/LICENSE-2.0.

This Source Code may also be made available under the following
Secondary Licenses when the conditions for such availability set
forth in the Eclipse Public License, v. 2.0 are satisfied: GNU
General Public License, version 2 with the GNU Classpath
Exception [1] and GNU General Public License, version 2 with the
OpenJDK Assembly Exception [2].
This Source Code may also be made available under the following
Secondary Licenses when the conditions for such availability set
forth in the Eclipse Public License, v. 2.0 are satisfied: GNU
General Public License, version 2 with the GNU Classpath
Exception [1] and GNU General Public License, version 2 with the
OpenJDK Assembly Exception [2].

[1] https://www.gnu.org/software/classpath/license.html
[2] http://openjdk.java.net/legal/assembly-exception.html
[1] https://www.gnu.org/software/classpath/license.html
[2] http://openjdk.java.net/legal/assembly-exception.html

SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
-->

<module>

<exports group="all">
<export name="JVM_OnUnload"/>
<export name="JVM_OnLoad"/>
Expand All @@ -49,16 +46,13 @@
<makefilestub data="UMA_TREAT_WARNINGS_AS_ERRORS=1"/>
<makefilestub data="UMA_ENABLE_ALL_WARNINGS=1"/>
<makefilestub data="CFLAGS+=-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1"/>
<makefilestub data="trclog.o: CFLAGS+=-U_FORTIFY_SOURCE">
<include-if condition="spec.linux_aarch64.*"/>
</makefilestub>
</makefilestubs>
<libraries>
<library name="j9util"/>
<library name="j9utilcore"/>
<library name="j9avl" type="external"/>
<library name="j9hashtable" type="external"/>
<library name="j9pool" type="external"/>
<library name="j9hashtable" type="external"/>
<library name="j9pool" type="external"/>
<library name="j9thr"/>
<library name="j9hookable"/>
<library name="j9stackmap"/>
Expand Down
69 changes: 37 additions & 32 deletions runtime/rastrace/trclog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,10 +885,12 @@ static UtTraceBuffer *
getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
{
UtTraceBuffer *nextBuf = NULL;
UtTraceBuffer *trcBuf;
int32_t newBuffer = FALSE;
uint32_t typeFlags = UT_TRC_BUFFER_ACTIVE | UT_TRC_BUFFER_NEW;
uint64_t writePlatform, writeSystem;
UtTraceBuffer *trcBuf = NULL;
int32_t newBuffer = FALSE;
uint32_t typeFlags = UT_TRC_BUFFER_ACTIVE | UT_TRC_BUFFER_NEW;
uint64_t writePlatform = 0;
uint64_t writeSystem = 0;
char *recordThreadName = NULL;

PORT_ACCESS_FROM_PORT(UT_GLOBAL(portLibrary));

Expand All @@ -898,26 +900,24 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)

if (oldBuf != NULL) {
/*
* Update write timestamp in case of a dump being taken before
* the buffer is ever written to disk for any reason
* Update write timestamp in case of a dump being taken before
* the buffer is ever written to disk for any reason.
*/
oldBuf->record.writeSystem = writeSystem;
oldBuf->record.writePlatform = writePlatform;

/* Null the thread reference as there's no guaranty it's valid after this point */
/* Null the thread reference as there's no guarantee it's valid after this point. */
oldBuf->thr = NULL;

if (UT_GLOBAL(traceInCore)) {
/*
* In core trace mode so reuse existing buffer, wrapping to the top
* In core trace mode so reuse existing buffer, wrapping to the top.
*/
trcBuf = oldBuf;

goto out;

} else {
/*
* External trace mode
* External trace mode.
*/
nextBuf = oldBuf->next;

Expand All @@ -931,7 +931,7 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
}

/* Set up nextBuf */
/* it's okay to set the global buffers here and initialize later because the entire
/* It's okay to set the global buffers here and initialize later because the entire
* function call's under the trace lock so nothing can be written to it until we release.
*/
if (bufferType == UT_NORMAL_BUFFER) {
Expand Down Expand Up @@ -964,7 +964,7 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)

oldBuf->lostCount += 1;

/* it's okay to set the global buffers here and initialize later because the entire
/* It's okay to set the global buffers here and initialize later because the entire
* function calls under the trace lock so nothing can be written to it until we release.
*/
if (bufferType == UT_NORMAL_BUFFER) {
Expand All @@ -988,14 +988,14 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
* Reuse buffer if there is one
*/
omrthread_monitor_enter(UT_GLOBAL(freeQueueLock));

trcBuf = UT_GLOBAL(freeQueue);
if (NULL != trcBuf) {
UT_GLOBAL(freeQueue) = trcBuf->next;
}

omrthread_monitor_exit(UT_GLOBAL(freeQueueLock));

if (trcBuf != NULL) {
DBG_ASSERT(trcBuf->queueData.next == NULL || trcBuf->queueData.next == CLEANING_MSG_FLAG);
DBG_ASSERT(trcBuf->queueData.referenceCount == 0);
Expand All @@ -1019,13 +1019,13 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)

newBuffer = TRUE;
trcBuf = j9mem_allocate_memory(UT_GLOBAL(bufferSize) +
offsetof(UtTraceBuffer, record), OMRMEM_CATEGORY_TRACE );
offsetof(UtTraceBuffer, record), OMRMEM_CATEGORY_TRACE);

if (trcBuf == NULL) {
if (UT_GLOBAL(dynamicBuffers) == TRUE) {
UT_GLOBAL(dynamicBuffers) = FALSE;
/* Native memory allocation failure, falling back to nodynamic trace settings. */
j9nls_printf(PORTLIB, J9NLS_WARNING | J9NLS_STDERR, J9NLS_TRC_NODYNAMIC_FALLBACK );
j9nls_printf(PORTLIB, J9NLS_WARNING | J9NLS_STDERR, J9NLS_TRC_NODYNAMIC_FALLBACK);
trcBuf = getTrcBuf(thr, oldBuf, bufferType);
}

Expand All @@ -1036,14 +1036,13 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
UT_DBGOUT(1, ("<UT> Allocated buffer %i " UT_POINTER_SPEC ", queue tail is "UT_POINTER_SPEC"\n", UT_GLOBAL(allocatedTraceBuffers), trcBuf, UT_GLOBAL(outputQueue).tail));
}


/*
* Initialize buffer for this thread
* Initialize buffer for this thread.
*/
UT_DBGOUT(5, ("<UT> Buffer " UT_POINTER_SPEC " obtained for thread " UT_POINTER_SPEC "\n",
trcBuf, thr));
trcBuf, thr));
initHeader(&trcBuf->header, UT_TRACE_BUFFER_NAME,
UT_GLOBAL(bufferSize) + offsetof(UtTraceBuffer, record));
UT_GLOBAL(bufferSize) + offsetof(UtTraceBuffer, record));
trcBuf->next = NULL;
trcBuf->lostCount = 0;
trcBuf->bufferType = bufferType;
Expand All @@ -1055,29 +1054,36 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
trcBuf->queueData.subscriptions = 0;
trcBuf->thr = NULL;

/*
* Get a pointer to where the thread name begins in a record. We don't
* just use (trcBuf->record.threadName) because threadName is declared
* as an array of length one and we're almost certainly writing more
* than one character there. Some compilers track the length of the
* target for strcpy, etc.: we want to avoid warnings due to the
* apparent lack of space (we've made sure there's enough room).
*/
recordThreadName = offsetof(UtTraceRecord, threadName) + (char *)(void *)&trcBuf->record;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to cast twice here? I don't think (void *) is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not, but I think it's safer with the extra cast as it should make it clear to the compiler that there's no relationship between the pointer &trcBuf->record and the result of the casts.

Consider an analogy with Java: You'll get a compile error trying to cast a String to an Integer, but you can write (Integer)(Object)"oops" which compiles just fine (though you'll get a ClassCastException at runtime). The cast to Object is comparable to the cast to void * (and there's no exception in C).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this particular part of the change will be unnecessary once eclipse/omr#5444 is merged.


if (bufferType == UT_NORMAL_BUFFER) {
trcBuf->record.threadId = (uint64_t)(uintptr_t)(*thr)->id;
trcBuf->record.threadSyn1 = (uint64_t)(uintptr_t)(*thr)->synonym1;
trcBuf->record.threadSyn2 = (uint64_t)(uintptr_t)(*thr)->synonym2;
strncpy(trcBuf->record.threadName, (*thr)->name,
UT_MAX_THREAD_NAME_LENGTH);
trcBuf->record.threadName[UT_MAX_THREAD_NAME_LENGTH] = '\0';
strncpy(recordThreadName, (*thr)->name, UT_MAX_THREAD_NAME_LENGTH);
recordThreadName[UT_MAX_THREAD_NAME_LENGTH] = '\0';
(*thr)->trcBuf = trcBuf;
} else if (bufferType == UT_EXCEPTION_BUFFER) {
trcBuf->record.threadId = 0;
trcBuf->record.threadSyn1 = 0;
trcBuf->record.threadSyn2 = 0;

strcpy(trcBuf->record.threadName, UT_EXCEPTION_THREAD_NAME);
strcpy(recordThreadName, UT_EXCEPTION_THREAD_NAME);
UT_GLOBAL(exceptionTrcBuf) = trcBuf;
}

trcBuf->record.firstEntry = offsetof(UtTraceRecord, threadName) +
(int32_t)strlen(trcBuf->record.threadName) + 1;
trcBuf->record.firstEntry = offsetof(UtTraceRecord, threadName)
+ (int32_t)strlen(recordThreadName) + 1;

/*
* Update circular buffer list for external trace
* Update circular buffer list for external trace.
*/

if (oldBuf != NULL) {
Expand All @@ -1088,12 +1094,11 @@ getTrcBuf(UtThreadData **thr, UtTraceBuffer * oldBuf, int bufferType)
}
}


out:

trcBuf->flags = typeFlags;

/* we reset the contents of the buffer first so that even if we're in the middle of flushing/writing this buffer
/* We reset the contents of the buffer first so that even if we're in the middle of flushing/writing this buffer
* the record maintains it's consistency.
*/
trcBuf->record.nextEntry = trcBuf->record.firstEntry;
Expand Down