stevedlawrence commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504245804
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SpecifiedLengthParsers.scala
##########
@@ -90,7 +90,9 @@ sealed abstract class SpecifiedLengthParserBase(
Assert.invariant(dis eq pState.dataInputStream)
val bitsToSkip = finalEndPos0b - dis.bitPos0b
Assert.invariant(bitsToSkip >= 0) // if this is < 0, then the parsing of
children went past the limit, which it isn't supposed to.
- if (bitsToSkip > 0) {
+ if (bitsToSkip > Int.MaxValue) {
+ PE(pState, "Data too large, consider using blobs instead:
https://cwiki.apache.org/confluence/display/DAFFODIL/Proposal%3A+Binary+Large+Objects";)
+ } else if (bitsToSkip > 0) {
Review comment:
I'm concerned there are other places we might need to deal with these
kinds of overflow issues?
For example, all the binary number parsers have a getBitLength funciton that
returns an int. I would never expect a binary number to have such a large
length that it overflows, but if there were bad data or a bad dfdl:length
expression, it could definiately happen. If we're lucky it overflows to a
negative number and hopefully we get an error message, but if it overflows to
some positive number we're just going to get unexpected behavior.
I'm not sure of a good way to handle this cleanly, since we deal with
lengths all over the place. Just grepping for getBitLength shows something lke
50 different implementations across different parsers and unparsers. I suspect
we're not handling overflow properly in many of them. And getLengthInBits shows
up as well. There are probably other length calcualtions that don't use either
of these.
##########
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/HexBinaryLengthParsers.scala
##########
@@ -36,6 +36,9 @@ sealed abstract class HexBinaryLengthParser(override val
context: ElementRuntime
val nBits = getLengthInBits(start).toInt
if (nBits == 0) {
currentElement.setDataValue(zeroLengthArray)
+ } else if (nBits < 0) {
+ // Integer overflow occurred, recommend using blob instead of hexBinary
+ PE(start, "Data is too large for xs:hexBinary. Consider using blobs
instead:
https://cwiki.apache.org/confluence/display/DAFFODIL/Proposal%3A+Binary+Large+Objects";)
Review comment:
I'm a little hesitant to include this link, only because it's very
possible this link might change in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
