ppkarwasz commented on code in PR #1495:
URL: https://github.com/apache/commons-lang/pull/1495#discussion_r2540890771
##########
src/main/java/org/apache/commons/lang3/ClassUtils.java:
##########
@@ -1523,13 +1540,44 @@ public static Class<?> primitiveToWrapper(final
Class<?> cls) {
*
* @param className the class name.
* @return the converted name.
- * @throws NullPointerException if the className is null.
+ * @throws NullPointerException if the className is null.
* @throws IllegalArgumentException Thrown if the class name represents an
array with more dimensions than the JVM supports, 255.
+ * @throws IllegalArgumentException Thrown if the class name length is
greater than 65,535.
+ * @see <a
href="https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.4.1";>JVM:
Array dimension limits in JVM Specification
+ * CONSTANT_Class_info</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-6.html#jls-6.7";>JLS:
Fully Qualified Names and Canonical Names</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-13.html#jls-13.1";>JLS:
The Form of a Binary</a>
*/
private static String toCanonicalName(final String className) {
String canonicalName = StringUtils.deleteWhitespace(className);
Objects.requireNonNull(canonicalName, "className");
+ if (canonicalName.isEmpty()) {
+ throw new IllegalArgumentException("Class name is empty");
+ }
+ final String encodedArrayOpen = "[";
+ final String encodedClassNameStart = "L";
+ final String encodedClassNameEnd = ";";
+ final boolean encodedName = canonicalName.startsWith(encodedArrayOpen)
&& canonicalName.endsWith(encodedClassNameEnd);
+ if (encodedName) {
+ final int arrIdx = canonicalName.indexOf(encodedClassNameStart);
+ if (arrIdx > MAX_JVM_ARRAY_DIMENSION) {
+ throw new IllegalArgumentException("Array dimension greater
than JVM specification maximum of 255.");
+ }
+ if (arrIdx < 0) {
+ throw new IllegalArgumentException("Expected 'L' after '[' for
an array style string.");
+ }
+ final int cnLen = canonicalName.length() - (arrIdx + 2); //
account for the ending ';'
+ if (cnLen > MAX_CLASS_NAME_LENGTH) {
+ throw new IllegalArgumentException(String.format("Class name
greater than maxium length %,d", MAX_CLASS_NAME_LENGTH));
+ }
+ }
final String arrayMarker = "[]";
+ final int arrIdx = canonicalName.indexOf(arrayMarker);
+ // The class name length without array markers.
+ final int cnLen = arrIdx > 0 ? arrIdx : canonicalName.length();
+ if (cnLen > MAX_CLASS_NAME_LENGTH && !encodedName) {
+ throw new IllegalArgumentException(String.format("Class name
greater than maxium length %,d", MAX_CLASS_NAME_LENGTH));
+ }
Review Comment:
Here I see an opportunity to improve the highly inefficient code below that
creates a substring each time the name ends with `[]`.
Unless there is garbage after the first `[]`, the rest of the string must be
a sequence of `[]`, so we can just assume that the dimension of the array is
`(canonicalName - cnLen) / 2`.
##########
src/main/java/org/apache/commons/lang3/ClassUtils.java:
##########
@@ -1523,13 +1540,44 @@ public static Class<?> primitiveToWrapper(final
Class<?> cls) {
*
* @param className the class name.
* @return the converted name.
- * @throws NullPointerException if the className is null.
+ * @throws NullPointerException if the className is null.
* @throws IllegalArgumentException Thrown if the class name represents an
array with more dimensions than the JVM supports, 255.
+ * @throws IllegalArgumentException Thrown if the class name length is
greater than 65,535.
+ * @see <a
href="https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.4.1";>JVM:
Array dimension limits in JVM Specification
+ * CONSTANT_Class_info</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-6.html#jls-6.7";>JLS:
Fully Qualified Names and Canonical Names</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-13.html#jls-13.1";>JLS:
The Form of a Binary</a>
*/
private static String toCanonicalName(final String className) {
String canonicalName = StringUtils.deleteWhitespace(className);
Objects.requireNonNull(canonicalName, "className");
+ if (canonicalName.isEmpty()) {
+ throw new IllegalArgumentException("Class name is empty");
+ }
+ final String encodedArrayOpen = "[";
+ final String encodedClassNameStart = "L";
+ final String encodedClassNameEnd = ";";
+ final boolean encodedName = canonicalName.startsWith(encodedArrayOpen)
&& canonicalName.endsWith(encodedClassNameEnd);
+ if (encodedName) {
+ final int arrIdx = canonicalName.indexOf(encodedClassNameStart);
+ if (arrIdx > MAX_JVM_ARRAY_DIMENSION) {
+ throw new IllegalArgumentException("Array dimension greater
than JVM specification maximum of 255.");
+ }
+ if (arrIdx < 0) {
+ throw new IllegalArgumentException("Expected 'L' after '[' for
an array style string.");
+ }
+ final int cnLen = canonicalName.length() - (arrIdx + 2); //
account for the ending ';'
+ if (cnLen > MAX_CLASS_NAME_LENGTH) {
+ throw new IllegalArgumentException(String.format("Class name
greater than maxium length %,d", MAX_CLASS_NAME_LENGTH));
+ }
+ }
Review Comment:
Nice refactoring! :100:
Should we push it further? If the name starts with `[`, then it must be a
field descriptor ([JVMS
4.3.2](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.3.2))
and follow the regex `\[+([BCDFIJSZ]|L[^\[;]+;)`.
The most we can do for the user is to replace the `/` separator used
internally in class files with `.`, which is only separator accepted by
`Class.forName()`.
Do we also want to accept `[Lint;` instead of `[LZ;`?
##########
src/main/java/org/apache/commons/lang3/ClassUtils.java:
##########
@@ -1523,13 +1540,44 @@ public static Class<?> primitiveToWrapper(final
Class<?> cls) {
*
* @param className the class name.
* @return the converted name.
- * @throws NullPointerException if the className is null.
+ * @throws NullPointerException if the className is null.
* @throws IllegalArgumentException Thrown if the class name represents an
array with more dimensions than the JVM supports, 255.
+ * @throws IllegalArgumentException Thrown if the class name length is
greater than 65,535.
+ * @see <a
href="https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.4.1";>JVM:
Array dimension limits in JVM Specification
+ * CONSTANT_Class_info</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-6.html#jls-6.7";>JLS:
Fully Qualified Names and Canonical Names</a>
+ * @see <a
href="https://docs.oracle.com/javase/specs/jls/se25/html/jls-13.html#jls-13.1";>JLS:
The Form of a Binary</a>
*/
private static String toCanonicalName(final String className) {
String canonicalName = StringUtils.deleteWhitespace(className);
Objects.requireNonNull(canonicalName, "className");
+ if (canonicalName.isEmpty()) {
+ throw new IllegalArgumentException("Class name is empty");
+ }
+ final String encodedArrayOpen = "[";
+ final String encodedClassNameStart = "L";
+ final String encodedClassNameEnd = ";";
+ final boolean encodedName = canonicalName.startsWith(encodedArrayOpen)
&& canonicalName.endsWith(encodedClassNameEnd);
+ if (encodedName) {
+ final int arrIdx = canonicalName.indexOf(encodedClassNameStart);
+ if (arrIdx > MAX_JVM_ARRAY_DIMENSION) {
+ throw new IllegalArgumentException("Array dimension greater
than JVM specification maximum of 255.");
+ }
+ if (arrIdx < 0) {
+ throw new IllegalArgumentException("Expected 'L' after '[' for
an array style string.");
+ }
+ final int cnLen = canonicalName.length() - (arrIdx + 2); //
account for the ending ';'
+ if (cnLen > MAX_CLASS_NAME_LENGTH) {
+ throw new IllegalArgumentException(String.format("Class name
greater than maxium length %,d", MAX_CLASS_NAME_LENGTH));
+ }
+ }
Review Comment:
Nice refactoring! :100:
Should we push it further? If the name starts with `[`, then it must be a
field descriptor ([JVMS
4.3.2](https://docs.oracle.com/javase/specs/jvms/se25/html/jvms-4.html#jvms-4.3.2))
and follow the regex `\[+([BCDFIJSZ]|L[^\[;]+;)`.
The most we can do for the user is to replace the `/` separator used
internally in class files with `.`, which is only separator accepted by
`Class.forName()`.
Do we also want to accept `[Lint;` instead of `[LZ`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
