Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a problem when hexStrLength is odd #329

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sword03
Copy link

@sword03 sword03 commented Jan 7, 2021

I met problems while I referred to the library.

  • Problem 1
  const a = CryptoJS.enc.Hex.parse('d6021ef5d7cccd55cda318fe2bd47334bac0b699e4a6676f8d941f7706d3820')
  const expected_a = a.toString(CryptoJS.enc.Hex)
  console.log('expected_a:', expected_a)
  
  The result is: 
  expected_a: d6021ef5d7cccd55cda318fe2bd47334bac0b699e4a6676f8d941f7706d38200
  • Problem 2
const a = CryptoJS.enc.Hex.parse('d6021ef5d7cccd55cda318fe2bd47334bac0b699e4a6676f8d941f7706d3821')
const expected_a = a.toString(CryptoJS.enc.Hex)
console.log('expected_a:', expected_a)

The result is: 
expected_a: d6021ef5d7cccd55cda318fe2bd47334bac0b699e4a6676f8d941f7706d38201

I reviewed the source code, then I located the bug:

image

It was supposed that hexStrLengh is even. The problem will recur if hexStrLengh is odd.

Finally, I fix the problem and commit the solution.

@OptimusPi
Copy link

To my understanding, a typical HEX should not be odd.. it should always be even. You could 'pad' either in the front or back, depending where you want the bytes shifted...and that is inherently ambiguous in general 🤷‍♂️

php decided to turn down a bug report for a similar behavior; and continue returning an error/warning when odd-length hex string is detected.

Not sure if it makes more sense to throw an error/warning here or to do what you've done in your PR.

@@ -414,6 +414,10 @@ var CryptoJS = CryptoJS || (function (Math, undefined) {
parse: function (hexStr) {
// Shortcut
var hexStrLength = hexStr.length;
if( hexStrLength % 2 === 1 ){
hexStr = '0' + hexStr

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hexStr = '0' + hexStr
hexStr = '0' + hexStr;

@@ -414,6 +414,10 @@ var CryptoJS = CryptoJS || (function (Math, undefined) {
parse: function (hexStr) {
// Shortcut
var hexStrLength = hexStr.length;
if( hexStrLength % 2 === 1 ){
hexStr = '0' + hexStr
hexStrLength = hexStrLength + 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hexStrLength = hexStrLength + 1
hexStrLength = hexStrLength + 1;

Copy link

@OptimusPi OptimusPi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have a unit test proving this works for even and odd strings.

@trasherdk
Copy link

How do you get an odd length hex string, and how do you decide if start or end need padding?

This feels more like a Throw situation than a let's try to guess a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants