Fix writeUint

[mvduin]

ASN.1 BER encodes integers 0 .. 2**32-1 into 1 to 5 bytes:

00 .. 7f
00 80 .. 7f ff
00 80 00 .. 7f ff ff
00 80 00 00 .. 7f ff ff ff
00 80 00 00 00 .. 00 ff ff ff ff

The leading 00 byte is required when the second byte is 80..FF since without it the integer would be parsed as being negative.

In contrast, writeUint was producing the encodings:

00 00 00 00 .. 00 7f ff ff   (redundant leading bytes)
00 80 00 00 .. 7f ff ff ff   (ok)
80 00 00 00 .. ff 7f ff ff   (wrong value)
ff 80 00 00 .. ff ff ff ff   (wrong value and redundant leading bytes)

Note that redundant leading bytes are not permitted and will cause a conforming ASN.1 BER parser to reject the data.

[markabrahams]

Hi @mvduin - I wonder now that asn1-ber has fixed its writeInt() encodings, whether net-snmp needs the writeUint() function at all?

asn1-ber1's writeInt() does the number type check first thing, so that's not required now in net-snmp's writeUint(), and neither is the workaround for 5-byte encodings now that that's fixed.

So we're really only left with buffer.writeInt (value, type), which the net-snmp callers can call directly, like they do for asn1-ber's other write functions e.g. writeOID(), writeByte(), etc. These all get called directly, so it would seem consistent to do the same with writeInt(). I'm not across the history here, but it seems like writeUint() might have been some sort of workaround to avoid calling the asn1-ber writeint() anyway?

So we can easily just replace all the net-snmp callers of writeUint() with buffer.writeInt(). The asn1-ber fix wasn't available when you raised this PR, so we didn't have that option when you raised it, but now we do!

What do you think?

[mvduin]

Absolutely, you can just use writeInt and bump the dependency version in package.json. Perhaps a range check to ensure the number is actually in range of uint32 would be nice (although int32 is currently also lacking a range check).

Note that a convenient way to check that a value is an uint32 or int32 is using value === value >>> 0 (for uint32) or value === value >> 0 (for int32) respectively, which both checks that value is an integer (hence in particular a number) and that it's within the appropriate range.

[mvduin]

similarly readInt and readUint can now just be replaced by asn1-ber's readInt followed by either a range check (to catch non-conforming data such as that produced by previous versions of this module) or a cast to int32 (>> 0) or uint32 (>>> 0) respectively to be tolerant of malformed data.

[markabrahams]

I've just migrated/renamed readInt32(), readUint32(), writeInt32() and writeUint32() to all use asn1-ber again.

I've also implemented the range checks, which throw an error for values outside the expected range.

Changes are on master and published in version 3.8.0 of the npm.