I thought it was because gcc sees that "len" is always passed in as
strlen(str). It therefore knows without any doubt that you are doing
strncpy() on a nul terminated string, and are copying all but the nul.
However, I missed one case where parse_value() is called and len not
only is not computed by strlen(), but the string is a delimited one, not
nul terminated:
void GenDCmdArgument::read_value(const char* str, size_t len, TRAPS) {
...
parse_value(str, len, CHECK);
set_is_set(true);
}
Look at calls to read_value and you'll see it is a delimited string.
This seems to be the path for parsing actual specified arguments. The
other calls to parse_value() are all for default values, which are
always a single nul terminated string.
What's also odd is that the code for DCmdArgument<jlong>::parse_value()
is identical to DCmdArgument<jboolean>::parse_value() w.r.t. the
strncpy(), but does not produce a warning.
Chris
On 10/10/19 10:29 AM, Ioi Lam wrote:
We have strncpy all over the VM. Why is this the only one that's
causing problems with gcc?
We have one instance in the same file:
void StringArrayArgument::add(const char* str, size_t len) {
if (str != NULL) {
char* ptr = NEW_C_HEAP_ARRAY(char, len+1, mtInternal);
strncpy(ptr, str, len);
ptr[len] = 0;
_array->append(ptr);
}
}
that looks rather similar to the troubling case:
char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
strncpy(buf, str, len);
buf[len] = '\0';
To me, this seems like a bug in gcc 9.2.1, which is the cutting edge
of gcc releases. Maybe we should just avoid supporting gcc 9.2 until
this has been fixed?
Thanks
- Ioi
On 10/10/19 8:58 AM, Chris Plummer wrote:
On 10/10/19 1:07 AM, Yasumasa Suenaga wrote:
On 2019/10/10 16:42, David Holmes wrote:
On 10/10/2019 5:25 pm, serguei.spit...@oracle.com wrote:
On 10/9/19 23:39, Chris Plummer wrote:
I think the +1 gets rid of the warning, but it also means there
is no need for the following line that sets buf[len].
Yes, it was my guess.
Then I agree this statement must be removed: buf[len] = '\0';
I don't think so!
Given this description:
template <> void DCmdArgument<bool>::parse_value(const char* str,
size_t len, TRAPS) {
// len is the length of the current token starting at str
it is not evident to me that we are even dealing with a
NUL-terminated str. Rather str is the start of the next token, with
length len, after which may be another token and so forth. So we
may be passing in:
str = "token1 token2"
len = 6
so we still need the explicit buf[len]='\0'
I missed it, thanks!
I think we can fix it as below.
-------------
diff -r b4f2e13d20ea src/hotspot/share/services/diagnosticArgument.cpp
--- a/src/hotspot/share/services/diagnosticArgument.cpp Wed Oct 09
19:38:11 2019 -0700
+++ b/src/hotspot/share/services/diagnosticArgument.cpp Thu Oct 10
17:04:09 2019 +0900
@@ -151,7 +151,7 @@
ResourceMark rm;
char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
- strncpy(buf, str, len);
memcpy(buf, str, len);
buf[len] = '\0';
Exceptions::fthrow(THREAD_AND_LOCATION,
vmSymbols::java_lang_IllegalArgumentException(),
"Boolean parsing error in command argument '%s'. Could not
parse: %s.\n", _name, buf);
-------------
I will upload webrev if it is ok.
That looks right, although it's odd that the memcpy line does not
have a + in front of it to show that it's an addition.
Chris
Thanks,
Yasumasa
David
Thanks,
Serguei
Chris
On 10/9/19 11:31 PM, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
The fix in src/hotspot/cpu/x86/macroAssembler_x86.hpp looks Okay.
But I don't understand why you need this fix in
src/hotspot/share/services/diagnosticArgument.cpp ?:
char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
- strncpy(buf, str, len);
+ strncpy(buf, str, len + 1);
buf[len] = '\0';
The buf[len] is set to '\0' anyway.
Thanks,
Serguei
On 10/9/19 22:55, Yasumasa Suenaga wrote:
Hi all,
Please review this change:
JBS: https://bugs.openjdk.java.net/browse/JDK-8232084
webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.00/
I tried to build OpenJDK on Fedora 30 x64 with GCC 9.2.1, but
it was failed in
macroAssembler_x86.hpp and diagnosticArgument.cpp .
(Please see JBS for details)
This change has passed tests on submit repo.
Thanks,
Yasumasa (ysuenaga)
