Repository navigation
Fix Int overflow in binary number lengths - #1760
stevedlawrence merged 1 commit into
Conversation
314072d to
40096aa
Compare
jadams-tresys
left a comment
jadams-tresys
left a comment
There was a problem hiding this comment.
+1
Just a couple places where we are converting back to .toInt that I want to make sure aren't a problem
|
|
||
| try { | ||
| val int = toPrimType(context, dis.getByteArray(nBits, start)) | ||
| val int = toPrimType(context, dis.getByteArray(nBits.toInt, start)) |
There was a problem hiding this comment.
Don't know if it matters or not, but there is still potential for an overflow/truncation here. For dis.getUnsignedLong it looks like there is a check to ensure that nBits is between 1 and 64, but there is no check for getByteArray
There was a problem hiding this comment.
I think it doesn't really matter in this specific case. The above checkBitLength will check that nBits fits within an int, so this nBits.toInt call shouldn't overflow and should create a valid int. And getByteArray works for any int value, we don't place any limits on how big of a byte array you can get, so shouldn't cause problems as long as it gets a valid int.
|
|
||
| if (nBits > 64) { | ||
| dos.putBigInt(asBigInt(value), nBits, primNumeric.isSigned, finfo) | ||
| dos.putBigInt(asBigInt(value), nBits.toInt, primNumeric.isSigned, finfo) |
There was a problem hiding this comment.
Couldn't this overflow as well or does checkMaxWidth ensure that nBits would be able to be stored in an int?
There was a problem hiding this comment.
Yep, checkMaxWidth ensures nBits.toInt won't overflow
| if (binaryBooleanTrueRep.isDefined) binaryBooleanTrueRep.getULong | ||
| else ~binaryBooleanFalseRep | ||
| putNumber(dos, trueRep, nBits, state) | ||
| putNumber(dos, trueRep, nBits.toInt, state) |
There was a problem hiding this comment.
Do these putNumber calls need to worry about integer overflow/truncation?
There was a problem hiding this comment.
From what I can tell, putNumber, or the functions they call, will make sure the number it is putting fits withing th nBits parameter. The check that were we sometimes missing was that the original nBits Long value can be safely converted to an Int. In this specific case, we check tht above with the nBits < 1 || nBits > 32, so nBits.toInt should be safe here.
olabusayoT
left a comment
olabusayoT
left a comment
There was a problem hiding this comment.
Some questions about the toInts, and requests for comments...but looks good overall
getBitLength currently returns an Int, with implementations often using Long.toInt. This can lead to silent overflow/truncation (e.g. with dfdl:length runtime expressions). To resolve this, getBitLength is modified to return a Long so that no truncation needs to happen. Ultimately these lengths need to be converted to integers for the various IO functions, so additional functions are added to ensure we also check the lengths prior to casting to an int. For types that have unlimited lengths (e.g. xs:integer, xs:decimal, packed decimals) a new tunable is added to restrict binary number lengths to reasonable values. This tunable has an xs:int base so if the checks pass the cast from long to int should never overflow. Also replaces the various packed DelimiterUnparsers with the packed MinimumLengthUnparsers. The DelimiterUnparsers have getBitLength return 0 which fails the checks. The MimumLengthUnparers calculate the actual length of the packed fields, which can then be checked against the tunable. DAFFODIL-3088
40096aa to
bfaa50e
Compare
getBitLength currently returns an Int, with implementations often using Long.toInt. This can lead to silent overflow/truncation (e.g. with dfdl:length runtime expressions). To resolve this, getBitLength is modified to return a Long so that no truncation needs to happen.
Ultimately these lengths need to be converted to integers for the various IO functions, so additional functions are added to ensure we also check the lengths prior to casting to an int. For types that have unlimited lengths (e.g. xs:integer, xs:decimal, packed decimals) a new tunable is added to restrict binary number lengths to reasonable values. This tunable has an xs:int base so if the checks pass the cast from long to int should never overflow.
Also replaces the various packed DelimiterUnparsers with the packed MinimumLengthUnparsers. The DelimiterUnparsers have getBitLength return 0 which fails the checks. The MimumLengthUnparers calculate the actual length of the packed fields, which can then be checked against the tunable.
DAFFODIL-3088