Replace sprintf with snprintf in many places #20474

pshipton · 2024-10-31T21:46:19Z

There is one remaining place that will require a coordinated merge with OMR.

dsouzai

I believe in the compiler files you need to use TR::snprintfTrunc; @jdmpapin could you confirm? (Something to do with windows and snprintf not being defined by default).

jdmpapin · 2024-11-01T16:07:40Z

At call sites where we're really confident that the buffer is large enough, TR::snprintfNoTrunc would be better, since it asserts that there is no truncation. But for a blanket replacement I think it's fine not to try to make that determination

The point of TR::snprintfTrunc was to smooth over portability problems with MSVC. Apparently their snprintf has been fixed for some time:

Beginning with the UCRT in Visual Studio 2015 and Windows 10, snprintf is no longer identical to _snprintf. The snprintf behavior is now C99 standard conformant.

If we're OK limiting ourselves to versions of the MSVC runtime library that include this change, then plain snprintf will be fine. Otherwise, the compiler should still use TR::snprintfTrunc, and outside of the compiler you would probably want to take account of the fact that old MSVC snprintf can fail to null-terminate its output

pshipton · 2024-11-01T16:19:38Z

We've moved up to VS2022, and VS2013 and earlier are out of support so I think it's fine.

runtime/compiler/control/CompilationThread.cpp

runtime/compiler/control/J9CompilationStrategy.cpp

keithc-ca · 2024-11-01T17:36:25Z

runtime/compiler/control/J9CompilationStrategy.cpp

+                     curMsg += msgLen;
+                     maxMsgLen -= msgLen;
+                     }
+                     else


Please correct the indentation here.

keithc-ca · 2024-11-01T17:37:12Z

runtime/compiler/control/J9CompilationStrategy.cpp

+                     curMsg += msgLen;
+                     maxMsgLen -= msgLen;
+                     }
+                     else


indentation

keithc-ca · 2024-11-01T17:39:05Z

runtime/compiler/control/J9CompilationStrategy.cpp

+                     curMsg += msgLen;
+                     maxMsgLen -= msgLen;
+                     }
+                     else


indentation; also several places below

keithc-ca · 2024-11-01T18:25:33Z

runtime/vm/JFRChunkWriter.hpp

@@ -222,8 +222,9 @@ class VM_JFRChunkWriter {
 	writeIntermediateJFRChunkToFile()
 	{
 		UDATA written = 0;
-		char fileName[sizeof(intermediateChunkFileName) + 16 + sizeof(".jfr")];
-		sprintf(fileName, "%s%lX.jfr", intermediateChunkFileName, _vm->jfrState.jfrChunkCount);
+		size_t fileNameLen = sizeof(intermediateChunkFileName) + 16 + sizeof(".jfr");


const may be necessary here

keithc-ca · 2024-11-01T18:25:49Z

runtime/vm/JFRChunkWriter.hpp

-		sprintf(fileName, "%s%lX.jfr", intermediateChunkFileName, _vm->jfrState.jfrChunkCount);
+		size_t fileNameLen = sizeof(intermediateChunkFileName) + 16 + sizeof(".jfr");
+		char fileName[fileNameLen];
+		snprintf(fileName, fileNameLen, "%s%lX.jfr", intermediateChunkFileName, _vm->jfrState.jfrChunkCount);
 		UDATA len = _bufferWriter->getSize();
 		IDATA fd = j9file_open(fileName, EsOpenWrite | EsOpenCreate | EsOpenTruncate , 0666);


While you're here, please remove the space before the last comma.

keithc-ca · 2024-11-01T18:28:22Z

runtime/vm/linearswalk.c

@@ -396,16 +396,20 @@ lswRecordSlot(J9StackWalkState * walkState, const void * slotAddress, UDATA slot


 static char *
-lswDescribeObject(J9VMThread * vmThread, char * dataPtr, J9Class ** class, J9Object * object)
+lswDescribeObject(J9VMThread * vmThread, char * dataPtr, UDATA dataSize, J9Class ** class, J9Object * object)


Suggest size_t is preferable to UDATA here and in the signature of lswDescribeSlot().

keithc-ca · 2024-11-01T18:33:47Z

runtime/vm/linearswalk.c

@@ -427,21 +431,30 @@ lswDescribeSlot(J9VMThread * vmThread, J9StackWalkState * walkState, J9SWSlot *
 		case LSW_TYPE_INDIRECT_O_SLOT:
 		case LSW_TYPE_O_SLOT:
 			if (slot->data) {
-
+				char *newPtr = dataPtr;


NULL is just as good for an initial value here.

keithc-ca · 2024-11-01T18:34:29Z

runtime/vm/linearswalk.c


 				className = J9ROMCLASS_CLASSNAME(class->romClass);
-				copySize = (J9UTF8_LENGTH(className) <= dataSize) ? J9UTF8_LENGTH(className) : LSW_STRING_MAX - 1;
+				copySize = (J9UTF8_LENGTH(className) < dataSize) ? J9UTF8_LENGTH(className) : (dataSize > 0 ? dataSize - 1 : 0);


Missing parentheses for (dataSize - 1).

pshipton · 2024-11-01T20:13:25Z

I've pushed an update to address the initial review comments.

keithc-ca · 2024-11-01T20:33:43Z

runtime/compiler/env/VMJ9.cpp

@@ -2628,25 +2628,25 @@ TR_J9VMBase::printTruncatedSignature(char *sigBuf, int32_t bufLen, J9UTF8 *class
   int32_t sigLen = J9UTF8_LENGTH(className) + J9UTF8_LENGTH(name) + J9UTF8_LENGTH(signature)+2;
   if (sigLen < bufLen)
      {
-      sigLen = sprintf(sigBuf, "%.*s.%.*s%.*s", J9UTF8_LENGTH(className), utf8Data(className),
+      sigLen = snprintf(sigBuf, bufLen, "%.*s.%.*s%.*s", J9UTF8_LENGTH(className), utf8Data(className),


A cast should be added until then.

keithc-ca · 2024-11-01T20:48:39Z

runtime/compiler/env/VMJ9.cpp

@@ -7051,7 +7054,7 @@ TR_J9VM::sampleSignature(TR_OpaqueMethodBlock * aMethod, char *buf, int32_t bufL
   int32_t len = J9UTF8_LENGTH(className)+J9UTF8_LENGTH(name)+J9UTF8_LENGTH(signature)+3;
   char * s = len <= bufLen ? buf : (memory ? (char*)memory->allocateHeapMemory(len) : NULL);
   if (s)
-      sprintf(s, "%.*s.%.*s%.*s", J9UTF8_LENGTH(className), utf8Data(className), J9UTF8_LENGTH(name), utf8Data(name), J9UTF8_LENGTH(signature), utf8Data(signature));
+      snprintf(s, len, "%.*s.%.*s%.*s", J9UTF8_LENGTH(className), utf8Data(className), J9UTF8_LENGTH(name), utf8Data(name), J9UTF8_LENGTH(signature), utf8Data(signature));


A cast should be used here ((size_t)len).

keithc-ca · 2024-11-01T20:50:07Z

runtime/compiler/env/annotations/Annotations.cpp

-            sprintf(annNameBuffer,"parm(%d)",(flag & ANNOTATION_PARM_MASK) >> ANNOTATION_PARM_SHIFT);
-            TR_ASSERT( strlen(annNameBuffer) < ANNO_NAMEBUF_LEN, "buffer length somehow exceeded\n");
+            size_t len = snprintf(annNameBuffer, sizeof(annNameBuffer), "parm(%d)",(flag & ANNOTATION_PARM_MASK) >> ANNOTATION_PARM_SHIFT);
+            TR_ASSERT( len < sizeof(annNameBuffer), "buffer length somehow exceeded\n");


Please remove the space after the opening (.

keithc-ca · 2024-11-01T20:51:38Z

runtime/compiler/ilgen/Walker.cpp

@@ -5059,7 +5068,8 @@ TR_J9ByteCodeIlGenerator::runMacro(TR::SymbolReference * symRef)
            char *secondArgType = nextSignatureArgument(arrayElementType);
            int arrayElementTypeLength = secondArgType - arrayElementType;

-            char *expandedArgsSignature = (char*)comp()->trMemory()->allocateStackMemory(numElements * arrayElementTypeLength + 1);
+            size_t argsLen = numElements * arrayElementTypeLength + 1;


Missing parentheses:

size_t argsLen = (numElements * arrayElementTypeLength) + 1;

There is one remaining place that will require a coordinated merge with OMR. Signed-off-by: Peter Shipton <[email protected]>

pshipton · 2024-11-01T21:03:09Z

Pushed the new updates.

pshipton requested a review from keithc-ca October 31, 2024 21:46

pshipton requested a review from dsouzai as a code owner October 31, 2024 21:46

dsouzai reviewed Nov 1, 2024

View reviewed changes

pshipton force-pushed the sprintf branch from d0ba697 to 959917a Compare November 1, 2024 16:32

keithc-ca requested changes Nov 1, 2024

View reviewed changes

pshipton force-pushed the sprintf branch 2 times, most recently from 3dbe0ac to 6191097 Compare November 1, 2024 19:58

keithc-ca requested changes Nov 1, 2024

View reviewed changes

Replace sprintf with snprintf in many places

f047685

There is one remaining place that will require a coordinated merge with OMR. Signed-off-by: Peter Shipton <[email protected]>

pshipton force-pushed the sprintf branch from 6191097 to f047685 Compare November 1, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace sprintf with snprintf in many places #20474

Replace sprintf with snprintf in many places #20474

pshipton commented Oct 31, 2024

dsouzai left a comment •

edited

Loading

jdmpapin commented Nov 1, 2024

pshipton commented Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

pshipton commented Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

keithc-ca Nov 1, 2024

pshipton commented Nov 1, 2024

Replace sprintf with snprintf in many places #20474

Are you sure you want to change the base?

Replace sprintf with snprintf in many places #20474

Conversation

pshipton commented Oct 31, 2024

dsouzai left a comment • edited Loading

Choose a reason for hiding this comment

jdmpapin commented Nov 1, 2024

pshipton commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton commented Nov 1, 2024

dsouzai left a comment •

edited

Loading