RFR: 8187570: Comparison between pointer and char in MethodMatcher::canonicalize

[Yasumasa Suenaga]

Hi Erik,


> Anyways, I think Yasumasa also spotted this issue a while back, but I couldn't find a patch for it

I've pasted a patch in [1]. But I did not create webrev because Kim told me this issue was reported in JDK-8181503.
Currently, JDK-8181503 seems to be in progress.


> Yasumasa, you want to be author and/or contributor of this patch?

Please push this patch as your changeset :-) because I cannot access JPRT.
I'm thumbs up this change as a reviewer (ysuenaga).


Thanks,

Yasumasa


On 2017/09/15 18:24, Erik Helin wrote:
> Hi all,
> 
> when I compiled with gcc 7.1.1 it warned me about the following code:
> 
> bool MethodMatcher::canonicalize(char * line, const char *& error_msg) {
>    char* colon = strstr(line, "::");
>    bool have_colon = (colon != NULL);
>    if (have_colon) {
>      // Don't allow multiple '::'
>      if (colon + 2 != '\0') {
> 
> The problem is that colon is a pointer, so colon + 2 is a pointer, and then colon + 2 is compared '\0', which is a char :/
> 
> Anyways, I think Yasumasa also spotted this issue a while back, but I couldn't find a patch for it, so I quickly whipped one up:
> 
> http://cr.openjdk.java.net/~ehelin/8187570/00/
> 
> --- old/src/hotspot/share/compiler/methodMatcher.cpp    2017-09-15 10:43:49.430656504 +0200
> +++ new/src/hotspot/share/compiler/methodMatcher.cpp    2017-09-15 10:43:49.102654877 +0200
> @@ -96,7 +96,7 @@
>     bool have_colon = (colon != NULL);
>     if (have_colon) {
>       // Don't allow multiple '::'
> -    if (colon + 2 != '\0') {
> +    if (colon[2] != '\0') {
>         if (strstr(colon+2, "::")) {
>           error_msg = "Method pattern only allows one '::' allowed";
>           return false;
> 
> I was a little bit afraid of what would happen if line (the parameter to MethodMatcher::canonicalize) isn't properly null terminated, so I checked the callers, and it seems like all callers property null terminate the `line` argument.
> 
> Yasumasa, you want to be author and/or contributor of this patch?
> 
> Thanks,
> Erik
> 
> PS. First webrev and patch created with a consolidated forest, seems to be working fine :)