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

Encoding/decoding negative integers and bigger integers #90

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

killerstorm
Copy link

I added support for encoding & decoding negative integers. Existing implementation was producing erroneous values.

I've added tests for numbers at the edges: -1, -128, -129. No extensive testing was done.

Copy link
Owner

@indutny indutny left a comment

Choose a reason for hiding this comment

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

Generally looking very good! Thanks for submitting it. Left few comments that I hope to see fixed.

@@ -244,6 +244,10 @@ DERNode.prototype._decodeInt = function decodeInt(buffer, values) {
var raw = buffer.raw();
var res = new bignum(raw);

if ((raw[0] & 0x80) > 0) { // negative
Copy link
Owner

Choose a reason for hiding this comment

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

Should it be !== 0? Please consider putting the comment on the line before if.

Copy link
Owner

Choose a reason for hiding this comment

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

Another note: should we rather use bignum.fromTwos here, and avoid creating new bignum if the result is negative?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I can tell, bignum.fromTwos doesn't exist.

// check if sign bit is occupied
if ((bitLength % 8) == 0) {
// when number is of form 10..0 no correction is needed
if (num.zeroBits() + 1 != num.bitLength()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use spaces for indent.

// check if sign bit is occupied
if ((bitLength % 8) == 0) {
// when number is of form 10..0 no correction is needed
if (num.zeroBits() + 1 != num.bitLength()) {
Copy link
Owner

Choose a reason for hiding this comment

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

!==

var bitLength = num.bitLength();
var width = Math.ceil(bitLength / 8) * 8;
// check if sign bit is occupied
if ((bitLength % 8) == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

===

}
}
numArray = num.toTwos(width).toArray();
//console.log(numArray);
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove all commented code.

@indutny
Copy link
Owner

indutny commented Oct 31, 2017

I actually just added eslint to the package.json. So you can run npm run lint to fix the most of these issues.

@killerstorm
Copy link
Author

Thx, I also want to fix encoding JS integers > 2^32-1 while we are here. If you don't mind I'll redirect them to BN.

@killerstorm
Copy link
Author

Hi, I fixed lint issues and also added support for integers >= 2^31.

I don't see BN.fromTwos in BN library so I don't see an opportunity to handle it in a more efficient way.

Note that test "should encode OCSP request" fails, but it fails without my changes too.

@killerstorm killerstorm changed the title Encoding/decoding negative integers Encoding/decoding negative integers and bigger integers Dec 1, 2017
@blmalone
Copy link

Is this going to be merged?

@Kalmac
Copy link

Kalmac commented Oct 26, 2021

Is this only a conflict issue ? I need number support > 32 bits.

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.

4 participants