RFR (M): 8161224: CONSTANT_NameAndType_info permits references to illegal names and descriptors

[Rachel Protacio]

Great, thanks!

Rachel


On 9/6/2016 4:29 PM, Dmitry Dmitriev wrote:
> Looks good!
>
> Thanks,
> Dmitry
> ----- Original Message -----
> From: rachel.protacio at oracle.com
> To: dmitry.dmitriev at oracle.com
> Cc: hotspot-runtime-dev at openjdk.java.net
> Sent: Tuesday, September 6, 2016 11:23:11 PM GMT +03:00 Iraq
> Subject: Re: RFR (M): 8161224: CONSTANT_NameAndType_info permits 
> references to illegal names and descriptors
>
> That sounds fair to me. How's this?
>
>     @@ -4843,19 +4840,28 @@
>              }
>            }
>            else {
>     -        // 4900761: For class version > 48, any unicode is
>     allowed in class name.
>     +        // Skip leading 'L' and ignore first appearance of ';'
>              length--;
>              signature++;
>     -        while (length > 0 && signature[0] != ';') {
>     -          if (signature[0] == '.') {
>     -            classfile_parse_error("Class name contains illegal
>     character '.' in descriptor in class
>     file %s", CHECK_0);
>     -          }
>     -          length--;
>     -          signature++;
>     -        }
>     -        if (signature[0] == ';') { return signature + 1; }
>     -      }
>     -
>     +        char* c = strchr((char*) signature, ';');
>     +        // Format check signature
>     +        if (c != NULL) {
>     +          ResourceMark rm(THREAD);
>     +          int newlen = c - (char*) signature;
>     +          char* sig = NEW_RESOURCE_ARRAY(char, newlen + 1);
>     +          strncpy(sig, signature, newlen);
>     +          sig[newlen] = '\0';
>     +
>     +          bool legal = verify_unqualified_name(sig, newlen,
>     LegalClass);
>     +          if (!legal) {
>     +            classfile_parse_error("Class name contains illegal
>     character "
>     +                                  "in descriptor in class file %s",
>     +                                  CHECK_0);
>     +            return NULL;
>     +          }
>     +          return signature + newlen + 1;
>     +        }
>     +      }
>            return NULL;
>          }
>          case JVM_SIGNATURE_ARRAY:
>
> It builds fine and passes the jck tests in question.
> Rachel
>
> On 9/6/2016 4:14 PM, Dmitry Dmitriev wrote:
>
>     Hi Rachel,
>
>     Small question about resource mark: on line 4850 you allocate new resource array which is used only in verify_unqualified_name() call. Do we need resource mark in this block?
>
>     Thanks,
>     Dmitry
>
>     ----- Original Message -----
>     From:rachel.protacio at oracle.com
>     To:hotspot-runtime-dev at openjdk.java.net
>     Sent: Tuesday, September 6, 2016 11:01:45 PM GMT +03:00 Iraq
>     Subject: Re: RFR (M): 8161224: CONSTANT_NameAndType_info permits references to illegal names and descriptors
>
>     Hi Harold,
>
>     Thanks for the review! I've removed the resource mark. If no further
>     comments in the next hour, will commit.
>
>     Rachel
>
>     On 9/6/2016 3:29 PM, harold seigel wrote:
>
>         Hi Rachel,
>
>         The change looks good.  Just one nit:
>
>         No resource mark is needed before calling classfile_parse_error() at
>         line 4857.
>
>         (No new RFR needed.)
>
>         Thanks, Harold
>
>
>         On 9/1/2016 5:30 PM, Rachel Protacio wrote:
>
>             Hello!
>
>             Please review this fix, which addresses a few issues related to
>             incomplete format checking with NameAndType names and signatures.
>
>             First, the code that should have been format checking the strings in
>             later classfile versions was in fact just checking for periods, so
>             I've rewritten it to call verify_unqualified_name(). Second, the
>             checks were (depending on the version) only performed when referenced
>             through Methodref/Fieldref/InterfaceMethodref/InvokeDynamic, meaning
>             that non-referenced NameAndType bytecodes did not get checked like
>             they were supposed to. My change enforces the spec in both aspects.
>             To summarize:
>
>             The existing code had:
>             - strict checks for pre-5.0
>             - incomplete/non-spec-compliant checks for 5.0-and-later
>             - no checks for un-referenced NameAndType (A) names and (B)
>             6.0-and-earlier signatures.
>
>             My change has:
>             - the same strict checks for pre-5.0
>             - complete/spec-compliant checks for 5.0-and-later
>             - all checks moved to the NameAndType section so all names and
>             signatures will be checked regardless of whether NameAndType is
>             referenced.
>
>             Bug:https://bugs.openjdk.java.net/browse/JDK-8161224
>             Open webrev:http://cr.openjdk.java.net/~rprotacio/8161224.00/
>
>             Testing: The jck tests which had been failing for this bug now pass,
>             along with all other jck vm tests. Also tested with JPRT and RBT
>             hotspot_all and noncolo tests.
>
>             A compatibility request has been approved for this change.
>
>             Thank you,
>             Rachel
>
>