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

[Rachel Protacio]

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