gh-127740: For odd-length input to bytes.fromhex(...) change the error message to ValueError: fromhex() arg must be of even length

[srinivasreddy]

• Issue: The error message for odd-length input to bytes.fromhex is cryptic #127740

[hauntsaninja]

Can we avoid the upfront loop you're using to check this eagerly? E.g. check invalid_char vs hexlen in the error clause?

[srinivasreddy]

@hauntsaninja Please review again?

[picnixz]

I think hauntsaninja wanted the error check at the level of the error label, not during the parsing. Since the current code jumps to error in case of an odd-length input, is it possible to check that it's indeed odd-length when creating the error?

[cfbolz]

Yes, I agree with @picnixz and @hauntsaninja, the new if is done for every character pair now, which makes things less efficient.

I suggest moving the new check into the condition just below. if (bot >= 16) { is true if we are at the end of the string (because the null at the end is mapped to 37), so we can do the check in there.

[cfbolz]

No, that's not what I meant, sorry for being unclear. many invalid byte values are mapped to 37, not just null. So instead, you need to check for your original condition if (str >= end) {, but in the same place where you now added the if (bot == 37){ check.

[srinivasreddy]

@cfbolz what do you think now ?

[cfbolz]

@srinivasreddy code looks good now, imo. please add a news entry though.

[cfbolz]

Looks good to me

[hauntsaninja]

nit: would be nice to have a test case where the string is of even length, e.g. " aaa" and test cases where we check the error message for " aa a " / " aa a a "

[cfbolz]

sorry for going ahead and merging @hauntsaninja. I should have waited for more input from you.

[srinivasreddy]

#127818

[hauntsaninja]

oh all good, was just a nit and commits are cheap :-) thanks for reviewing!

[Kodiologist]

That was fast. Thanks.