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.

Sign up for GitHub

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

Jump to bottom

Replace Hex.toHexString in logging, toString etc to safer alternative #1032

Closed
zilm13 opened this issue Mar 13, 2018 · 8 comments
Closed

Replace Hex.toHexString in logging, toString etc to safer alternative #1032

zilm13 opened this issue Mar 13, 2018 · 8 comments
Labels
enhancement

Comments

@zilm13
Copy link
Collaborator

zilm13 commented Mar 13, 2018

Fails on null, pain.
@kishansagathiya
Copy link
Contributor

@zilm13 This looks like something simple that I can pick up, is it?

@zilm13
Copy link
Collaborator Author

zilm13 commented May 7, 2018

@kishansagathiya sure, check ByteUtil.toHexString, it exists, but not used in all hex logging.

@kishansagathiya
Copy link
Contributor

Tried few different libraries, failed to find any null safe hex encoder.

@zilm13
org.apache.commons.codec.binary.Hex.encodeHexString()
org.apache.soap.encoding.Hex.encode(null)
I guess spongycastle uses bouncycastle internally, but tried that anyway
org.bouncycastle.util.encoders.Hex.encode(null)

How about adding a null check before every call or create our own method that wraps this function with a null check,
I believe if null, we would be throwing illigalArgumentError right?

@zilm13
Copy link
Collaborator Author

zilm13 commented May 9, 2018

@kishansagathiya what you don't like in org.ethereum.util.ByteUtil.toHexStringwhich I mentioned before?

@kishansagathiya
Copy link
Contributor

@zilm13 I thought that is one more thing that is not null safe method :),

Anyway org.ethereum.util.ByteUtil.toHexString is perfect. I will use it.

@kishansagathiya
Copy link
Contributor

@zilm13
I believe you don't want to replace all Hex.toHexString() to ByteUtil.toHexUtil, right? Otherwise I can just do a replace all.

For something like

ethereumj/ethereumj-core/src/main/java/org/ethereum/config/SystemProperties.java

Line 687 in 4cdc799

props.setProperty("nodeIdPrivateKey", Hex.toHexString(key.getPrivKeyBytes()));

You would rather want to through an error instead of setting to null,
Right?

@zilm13
Copy link
Collaborator Author

zilm13 commented May 9, 2018

@kishansagathiya I want to change Hex.toHexString() to ByteUtil.toHexUtil in logging only. So we could see at least part of data in logs if smth is null (instead of Exception).

kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 9, 2018
@kishansagathiya
Issue ethereum#1032 Use null safe method for bytes to hex string conv…
29c1849 
…ersion

This is does not include all such cases
@kishansagathiya kishansagathiya mentioned this issue May 9, 2018
Merged
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 9, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
1c831ee 
This commit replaces use of
`org.spongycastle.util.encoders.Hex.toHexString()` (Not null safe) with
`org.ethereum.util.ByteUtil.toHexString()` (null safe) while logging.
This covers only logging in the implementation(`/src/main`) and doesn't
cover any logging in tests(`/src/tests`)

Issue ethereum#1032
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 11, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
654e0ab 
This commit replaces use of
`org.spongycastle.util.encoders.Hex.toHexString()` (Not null safe) with
`org.ethereum.util.ByteUtil.toHexString()` (null safe) while logging
and in all toString() methods.

This covers only logging in the implementation(`/src/main`) and doesn't
cover any logging in tests(`/src/tests`)

Issue ethereum#1032
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 16, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
11d6427 
This commit replaces use of
`org.spongycastle.util.encoders.Hex.toHexString()` (Not null safe) with
`org.ethereum.util.ByteUtil.toHexString()` (null safe) while logging
and in all toString() methods.

This covers only logging in the implementation(`/src/main`) and doesn't
cover any logging in tests(`/src/tests`)

Issue ethereum#1032
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 16, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
a8008c1 
`hint` and `contract` are only being used in logs. Changing the
toHexString function for them should not break anything.

Issue ethereum#1032
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 16, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
510ce2d 
Removed unused imports and other changes as asked for

Issue ethereum#1032
kishansagathiya added a commit to kishansagathiya/ethereumj that referenced this issue May 16, 2018
@kishansagathiya
Use null safe method to convert bytes to hexString
a5f4e74 
More conversions

Issue ethereum#1032
zilm13 added a commit that referenced this issue May 16, 2018
@zilm13
Merge pull request #1069 from kishansagathiya/issue_1032
e6a5dd6 
Issue #1032 Use null safe method for bytes to hex string conversion
@zilm13
Copy link
Collaborator Author

zilm13 commented May 16, 2018

resolved with #1069

@zilm13 zilm13 closed this as completed May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mkalinin @zilm13 @kishansagathiya