Read ConstantDynamic indices as unsigned shorts

[rootvector2]

ConstantDynamic(DataInput) reads bootstrap_method_attr_index and name_and_type_index with readShort(), so a CONSTANT_Dynamic entry whose index is 0x8000 or higher in an untrusted class sign-extends to a negative value that getBootstrapMethodAttrIndex()/getNameAndTypeIndex() then hand back as a constant-pool index. Both fields are u2 and the sibling ConstantInvokeDynamic already reads them with readUnsignedShort(). Found while sweeping the constant-pool parsers for signed reads of unsigned operands; switch the two reads to readUnsignedShort() to match.

[garydgregory]

Hello @rootvector2
What about these readShort() call sites:

• org.apache.bcel.classfile.Utility.codeToString(ByteSequence, ConstantPool, boolean)
• org.apache.bcel.generic.BranchInstruction.initFromFile(ByteSequence, boolean)
• org.apache.bcel.generic.IINC.initFromFile(ByteSequence, boolean)
• org.apache.bcel.generic.SIPUSH.initFromFile(ByteSequence, boolean)
• org.apache.bcel.util.CodeHTML.codeToHTML(ByteSequence, int)
• org.apache.bcel.util.CodeHTML.findGotos(ByteSequence, Code)

?

Thank you!
PR merged 🚀

[rootvector2]

Thanks for merging. I went through those six and they're all reading signed operands, not u2 indices, so readShort() is correct there:

• BranchInstruction.initFromFile, plus the goto/if branch cases in Utility.codeToString and CodeHTML read branch offsets, which are signed by definition (a backward branch is negative).
• SIPUSH.initFromFile and the T_SHORT operand case read the sipush immediate, a signed short that's sign-extended to int.
• IINC reads the increment const, also signed and sign-extended.

Where those same disassembly paths read an actual index they already use readUnsignedShort() (multianewarray's index, the wide iinc local index), so reading them unsigned would actually break negative offsets/immediates. ConstantDynamic was the one outlier because both its fields are u2 constant-pool indices. I found it by grepping the constant-pool parsers specifically rather than the operand readers, which is why those didn't come up.

[garydgregory]

Thank for checking @rootvector2
How about for u4?

[rootvector2]

No equivalent for u4. DataInput has no readUnsignedInt, so a u4 always lands in an int regardless and there is no one-line read-it-unsigned fix like there was for u2. Going through the readInt() sites, the fields with real unsigned u4 semantics are already covered:

• magic in ClassParser is only compared against JVM_CLASSFILE_MAGIC, so sign does not matter.
• code_length in Code goes through Args.requireU4(..., 0, MAX_CODE_SIZE, ...), which rejects a high-bit-set (negative) value since min is 0.
• attribute_length in Attribute.readAttribute flows into Unknown, gated by length > 0 and the MAX_LEN cap, so a negative length reads nothing rather than over-allocating.

The rest are genuinely signed s4 and have to stay readInt(): ConstantInteger's value, and the switch default offsets / low/high / match keys / jump offsets in Utility, CodeHTML, TABLESWITCH, LOOKUPSWITCH, Select, JSR_W, GOTO_W. So nothing to change for u4.

[garydgregory]

Thank you for the PR and review @rootvector2