RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Thomas Stuefe]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the use of it causing building failure.  The build could pass if warnings are disabled for codes that use sprintf method.  For the long run, the sprintf could be replaced with snprintf.  This patch is trying to check if snprintf could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   comment for snprintf_checked

I looked through all the code and found only some minor issues. I would appreciate more eyes on this, though.

I still think this would have been less work (for author and reviewers) had we converted the code to stringStream right away in the hotspot. stringStream takes care of a lot of this boilerplate stuff.

src/hotspot/os/bsd/attachListener_bsd.cpp line 260:

> 258:   // name ("load", "datadump", ...), and <arg> is an argument
> 259:   int expected_str_count = 2 + AttachOperation::arg_count_max;
> 260:   const int max_len = (ver_str_len + 1) + (AttachOperation::name_length_max + 1) +

This makes `max_len` a runtime variable, before it was a compile time constant. Below, we use it to create the buf array. IIRC creating an array with a variable runtime length is a non-standard compiler extension. I am surprised this even builds.

src/hotspot/share/adlc/output_c.cpp line 46:

> 44:   va_end(args);
> 45:   assert(result >= 0, "snprintf error");
> 46:   assert(static_cast<size_t>(result) < len, "snprintf truncated");

Question, what is this assert? The standard CRT assert? Will it fire always, or just in debug?

src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp line 311:

> 309:   for (int i = 0; i < len ; i++) {
> 310:     VMStructEntry vmField = JVMCIVMStructs::localHotSpotVMStructs[i];
> 311:     const size_t name_buf_size = strlen(vmField.typeName) + strlen(vmField.fieldName) + 3 /* "::" */;

nit, could you make this "+ 2 + 1" instead? That makes it a bit clearer that the last byte is for \0

src/hotspot/share/runtime/os.cpp line 102:

> 100: }
> 101: 
> 102: int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {

Can you please assert for buffer len > 0 and < some reasonable max, e.g. 1 gb? Protects us against neg overflows and other errors.

src/hotspot/share/runtime/os.cpp line 108:

> 106:   va_end(args);
> 107:   assert(result >= 0, "os::snprintf error");
> 108:   assert(static_cast<size_t>(result) < len, "os::snprintf truncated");

Can you please provide a printout for the truncated string? Proposal:

```assert(static_cast<size_t>(result) < len, "os::snprintf truncated ("%.200s...")", buf);```

src/hotspot/share/utilities/utf8.cpp line 227:

> 225:     } else {
> 226:       if (p + 6 >= end) break;      // string is truncated
> 227:       os::snprintf_checked(p, 7, "\\u%04x", c);  // counting terminating zero in

I had to think a while until I was sure this is correct :-) 
We are sure that c can never be > 0xFFFFFF (6 digits), right? But if that is false, code had been wrong before too.

-------------

PR: https://git.openjdk.org/jdk/pull/11115