Replace sprintf with snprintf in many places

[pshipton]

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]

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]

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

[pshipton]

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

[pshipton]

Pushed the new updates.

[keithc-ca]

Jenkins test sanity.functional alinux,amac,win jdk21

[keithc-ca]

I'll defer to someone from the JIT team to offer an opinion on the test failure:

        FAILED: testServerComesUpAfterClient
        java.lang.AssertionError: Expected an exit value of 0 or 143, got 134 instead.
        	at org.testng.AssertJUnit.fail(AssertJUnit.java:59)
        	at jit.test.jitserver.JITServerTest.destroyAndCheckProcess(JITServerTest.java:357)
        	at jit.test.jitserver.JITServerTest.testServerComesUpAfterClient(JITServerTest.java:572)

[pshipton]

@cjjdespres any insights about the test failure?

[cjjdespres]

It looks like the server process terminated prematurely, or incorrectly, at the end of that test - we expect the process to exit with 0 (success) or 143 (SIGTERM + 128). I think exit code 134 is SIGABRT + 128, so something called abort() before the server could shut down.

[cjjdespres]

In the console log for the failure, the server stdout/stderr that was dumped was:

        ////////////////////////////////////////////////////////////////////////
        ////
        //// JITServer is ready to accept incoming requests
        //// double free or corruption (fasttop)
        //// malloc_consolidate(): unaligned fastbin chunk detected
        ////////////////////////////////////////////////////////////////////////

I haven't seen that failure before.

[keithc-ca]

I haven't seen that failure before.

Do we think it is due to this change?

[cjjdespres]

The only thing I can see that's obviously JITServer-specific is the change in VMJ9Server.cpp and in JITServerRomClassHash::toString(). It's not obvious that the new code is wrong.

[pshipton]

Pushed just a rebase.

[pshipton]

Addressed the review comments, but in J9CompilationStrategy.cpp I'm going to effectively set _curMsg = _msg + sizeof(_msg); if snprintf returns < 0, to prevent further writes to the buffer.

[pshipton]

Pls re-review.

[pshipton]

Addressed the latest comments.

[keithc-ca]

one remaining place that will require a coordinated merge with OMR

Can you please elaborate?

I see a couple remaining uses of sprintf() in runtime/compiler/runtime/MetaDataDebug.cpp; the first could be replaced by strcpy(), the second by strcat().

[pshipton]

The function should be modified to pass in the size of the buffer, which requires an OMR change. Using strcpy() or strcat() isn't any better than using sprintf(), there is no restriction to the buffer size and a buffer overflow could occur.

[keithc-ca]

The function should be modified to pass in the size of the buffer, which requires an OMR change. Using strcpy() or strcat() isn't any better than using sprintf(), there is no restriction to the buffer size and a buffer overflow could occur.

It wasn't immediately obvious to me that the second call sprintf(indent, "%s ", indent); is guaranteed to terminate (it's writing to the same place it's reading from).

[pshipton]

I can remove the sprintf, but will do it in another PR.

[keithc-ca]

Jenkins test sanity win32 jdk8

[pshipton]

All nodes of label ‘[ci.role.build&&hw.arch.x86&&sw.os.windows](https://openj9-jenkins.osuosl.org/label/ci.role.build&&hw.arch.x86&&sw.os.windows/)’ are offline
There are no Windows build machines atm, I'm not sure when it will be fixed. The PR build would timeout in 12 hours or so. I killed it.

[keithc-ca]

The PR build would timeout in 12 hours or so. I killed it.

I was hoping to get the benefit of the pickiness of the Microsoft compiler - I'll try locally.

[keithc-ca]

Jenkins test sanity aix,alinux,amac,osx jdk21

[keithc-ca]

My local builds of jdk8 for 32- and 64-bit Windows were successful.

[pshipton]

The AIX failure to get through jdk_security2 is a known issue #19962

The mac failure is sun/security/krb5/auto/RefreshKrb5Config.java which is network related. It shows Caused by: java.net.SocketTimeoutException: Receive timed out. Unlikely it's related to this change. Various krb5 tests fail off and on, but aren't being tracked in any issues. I expect it can be re-run in a grinder and pass.
https://openj9-jenkins.osuosl.org/job/Grinder/3995/