On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan <xue...@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
