<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 5:17 PM, Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" class="">igor.ignatyev@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">strncpy(mn->methodName, szName, sizeof(mn->methodName) -1) is guaranteed to be null-terminated, it might produce truncated string if methodName is shorter than szName.</div></div></blockquote><div><br class=""></div><div>Unfortunately not. From man strncpy:</div><div><br class=""></div><div>"Warning: If there is no null byte among the first  n  bytes  of  src,  the  string placed in dest will not be null-terminated.”</div><div><br class=""></div><div>So no matter the length we’ll risk having an unterminated string.</div><div><br class=""></div><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">I'm actually not suggesting to replace the if you added, I'm suggesting to also add -1 to both strncpy, so gcc will be happy even if it becomes less smart.<br class=""></div></div></blockquote><div><br class=""></div><div>I took a stab at doing this "the C++ way”. Have a look and let me know what you think:</div><div><br class=""></div><div><a href="http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.01/open/webrev/" class="">http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.01/open/webrev/</a></div><div><br class=""></div><div>Cheers,</div><div>Mikael</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><div class="">-- Igor<br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 5:10 PM, Mikael Vidstedt <<a href="mailto:mikael.vidstedt@oracle.com" class="">mikael.vidstedt@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 4:48 PM, Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" class="">igor.ignatyev@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">> One could imagine wrapping the resource in a C++ class and have the destructor do the deallocation.<div class="">that sounds nice.</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><pre style="background-color: rgb(238, 238, 238);" class="">     strncpy(mn->methodName, szName, sizeof(mn->methodName));
     strncpy(mn->classSig, szSignature, sizeof(mn->classSig));</pre></blockquote><div class="">as we are doing this just to make gcc happier, wouldn't it be better to add -1 to sizeof?</div></div></div></div></blockquote><div class=""><br class=""></div><div class="">While that does silence the warning, it (still) has the issue of potentially leaving the resulting strings unterminated. We may know today that the strings in question are always shorter than the respective MethodName buffers, but it seems prudent to actually perform the checks to ensure that.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Mikael</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-- Igor</div><div class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 4:38 PM, Mikael Vidstedt <<a href="mailto:mikael.vidstedt@oracle.com" class="">mikael.vidstedt@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 4:12 PM, Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" class="">igor.ignatyev@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Mikael,<div class=""><br class=""></div><div class="">I really don't like us adding goto in cpp code, can we avoid that?</div></div></div></blockquote><div class=""><br class=""></div><div class="">One alternative is to make sure the Deallocate calls are injected in every if statement. I don’t think that’s any better.</div><div class=""><br class=""></div><div class="">Another alternative is to nest the if statements. I’m not a fan of that code style personally.</div><div class=""><br class=""></div><div class="">One could imagine wrapping the resource in a C++ class and have the destructor do the deallocation.</div><div class=""><br class=""></div><div class="">Let me know if you have other suggestions, and/or what you’d prefer.</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">I also doubt that any of mlvm tests are run in tier1, could you please also run :vmTestbase_vm_mlvm to test your fix?</div></div></blockquote><div class=""><br class=""></div><div class="">Will do.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Mikael</div><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-- Igor</div><div class=""><br class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Feb 13, 2019, at 3:45 PM, Mikael Vidstedt <<a href="mailto:mikael.vidstedt@oracle.com" class="">mikael.vidstedt@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html; charset=us-ascii" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class=""><br class=""></div>Please review this small fix which updates some uses of strncpy to address some GCC 8.x warnings.<div class=""><br class=""></div><div class="">Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8218937" class="">https://bugs.openjdk.java.net/browse/JDK-8218937</a></div><div class="">Webrev: <a href="http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.00/open/webrev/" class="">http://cr.openjdk.java.net/~mikael/webrevs/8218937/webrev.00/open/webrev/</a></div><div class=""><br class=""></div><div class="">GCC 8.2 is producing a warning for mlvmJvmtiUtils.cpp: <br class=""><br class="">In file included from test/hotspot/jtreg/vmTestbase/vm/mlvm/indy/func/jvmti/share/libIndyRedefineClass.cpp:31: <br class="">test/hotspot/jtreg/vmTestbase/vm/mlvm/share/mlvmJvmtiUtils.cpp: In function 'MethodName* getMethodName(jvmtiEnv*, jmethodID)': <br class="">test/hotspot/jtreg/vmTestbase/vm/mlvm/share/mlvmJvmtiUtils.cpp:80:12: error: 'char* strncpy(char*, const char*, size_t)' specified bound 256 equals destination size [-Werror=stringop-truncation] <br class="">     strncpy(mn->methodName, szName, sizeof(mn->methodName)); <br class="">     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~</div><div class=""><br class=""></div><div class="">Basically, gcc is pointing out that the resulting string is not necessarily going to be terminated. Explicitly checking the length of the source strings provides the information needed to guarantee that the strings will be terminated, and silences the warning.</div><div class=""><br class=""></div><div class="">Passes tier1.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Mikael</div><div class=""><br class=""></div></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br class=""></body></html>