Validate hexadecimal byte input - #1273
Conversation
Rejected odd-length and non-hexadecimal strings instead of producing truncated or corrupt byte arrays.
|
Hi @cragkhit, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
|
Hi @cragkhit, thanks for the contribution — the change is sound. The current code silently produces corrupted bytes for non-hex input (the old test even asserted It's a pity this was closed — was there any particular reason? Would you mind reopening it? |
|
@fengjiachun Thanks! I reviewed this change our automated approach made and found it didn't actually apply them as described in the Stack Overflow answer, so I closed it. Nonetheless, I'm glad that you considered it useful. I have reopened the PR! |
fengjiachun
left a comment
fengjiachun
left a comment
There was a problem hiding this comment.
Thanks for reopening! The fix itself looks good — a couple of small things before we can merge:
- CLA: please sign the Contributor License Agreement at http://cla.sofastack.tech — we can't merge without it. Once signed, the
cla:yeslabel will be synced automatically. - Javadoc: please add
@throws IllegalArgumentExceptionto thehexStringToByteArrayjavadoc describing the two new failure cases (odd length / non-hex character), which is the practice elsewhere in this package (e.g.Requires,ThreadPoolUtil).
Plus two inline nits below. Thanks again for the contribution!
| } | ||
| final int len = s.length(); | ||
| if ((len & 1) != 0) { | ||
| throw new IllegalArgumentException("Hex string must contain an even number of characters"); |
There was a problem hiding this comment.
Nit: the project convention for argument checks is Requires.requireTrue(cond, msg) from com.alipay.sofa.jraft.util.Requires (this class already uses Requires.requireNonNull in nextBytes). So this could be:
Requires.requireTrue((len & 1) == 0, "Hex string must contain an even number of characters");| final int high = Character.digit(s.charAt(i), 16); | ||
| final int low = Character.digit(s.charAt(i + 1), 16); | ||
| if (high < 0 || low < 0) { | ||
| throw new IllegalArgumentException("Hex string contains a non-hexadecimal character"); |
There was a problem hiding this comment.
Same here — Requires.requireTrue(high >= 0 && low >= 0, ...). Also consider including the offending position in the message, e.g. "Hex string contains a non-hexadecimal character at index " + i. Index only, please — better not to echo the character or the input string itself, since this util may handle key material and the message could end up in logs.
2 participants
Hi. We are researchers from Mahidol University, Thailand, and the State University of Ceará, Brazil, working on a research project for improving open-source projects by using the latest accepted answer from Stack Overflow that matched your code snippet. We found this recommendation for improving your code from https://stackoverflow.com/a/60266789.
Note: Our study is approved by the Institutional Review Board of Mahidol University. You can find the participant information sheet explaining this study https://drive.google.com/file/d/1ml5AqrtWQ9pnifTQyTFTcWQmwp6RuPA7/view?usp=sharing.
Proposed change
Rejected odd-length and non-hexadecimal strings instead of producing truncated or corrupt byte arrays.
Testing
mvn -q -pl jraft-core -Dtest=BytesUtilTest testResult: Passed