You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/10/13 19:57:49 UTC
[GitHub] [incubator-daffodil] jadams-tresys opened a new pull request #433: Alert user to use blobs instead of hexBinary
jadams-tresys opened a new pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433
The test files provided for this bug were bumping into the limits of
integer values, which we cannot create an array that is larger than
Int.MaxValue. In cases such as this, blobs should be used instead. Given
that this error is originating from the official NITF schema, we should
update the NITF schema to take advantage of blobs for large payloads.
DAFFODIL-2401
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504227875
##########
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
Review comment:
We're depending on 32-bit integer overflow? That's very unreliable. We should do math on Long type, and check the bound, and explicitly return -1 to indicate too big.
Or, if we are doing this right and signaling the overflow of the max length by -1 then the comment should change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504633049
##########
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 understand the hesitancy, but I figured that most people wouldn't be familiar with blobs and thought that the link might be the best way to point them in the right direction.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504628161
##########
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:
Hm, would a better fix be to modify the various get functions to use Longs, but then throw a ParseError inside the get functions if the value ends up being larger than Int.MaxValue?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504737632
##########
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
Review comment:
This has been addressed in my most recent commit
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504844496
##########
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:
Agreed, this would be a better link.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504888346
##########
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:
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504737300
##########
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 pushed up changes that I think cover all the getByteArray issues. As for the binaryNumberParsers, I think it might be worth making its own ticket that can be dealt with later as it is a much less likely situation to occur.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] jadams-tresys merged pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
jadams-tresys merged pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504810993
##########
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:
If you want to include a link, we should create an Apache short link that can be kept up to date.
I just created: https://s.apache.org/daffodil-blob-feature for this if we want to keep the link.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #433: Alert user to use blobs instead of hexBinary
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #433:
URL: https://github.com/apache/incubator-daffodil/pull/433#discussion_r504844261
##########
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:
Yeah, agreed this is too much to fix for now. I think we really do need to come up with a comprehensive way to ensure we dont' have any overflow issues, but it's fine to create a new bug and handle that more completely later.
----------------------------------------------------------------
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:
users@infra.apache.org