On Wed, 7 Dec 2022 08:46:45 GMT, Thomas Stuefe <[email protected]> wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comment for snprintf_checked
>
> 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?
The assert is defined in adlc.hpp, as follow:
`#define assert(cond, msg) { if (!(cond)) { fprintf(stderr, "assert fails %s
%d: %s\n", __FILE__, __LINE__, msg); abort(); }}
`
I think it will fire always.
> 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.
I may be hesitate for the checking. The behavior maybe unclear if a size_t
value could be negative and if it is possible the max limit is too small in
practice. If it is necessary, the svnprintf() could do that instead.
> 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);```
I'm not sure if there is sensitive information in the buf. Dumping the string
may result in sensitive information leaking.
> 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.
I think so as it used to work.
-------------
PR: https://git.openjdk.org/jdk/pull/11115
