You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "mike-mcgann (via GitHub)" <gi...@apache.org> on 2023/02/28 18:33:47 UTC
[GitHub] [daffodil] mike-mcgann opened a new pull request, #974: Add checks for bit lengths
mike-mcgann opened a new pull request, #974:
URL: https://github.com/apache/daffodil/pull/974
Daffodil was allowing bit lengths on binary types that exceed the width of the type even though this is not allowed by the DFDL specification. This change adds a check at compile time to see if the bit length value is valid when specified as a constant and at runtime when the bit length is specified an expression.
[DAFFODIL-1704](https://issues.apache.org/jira/browse/DAFFODIL-1704)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann commented on a diff in pull request #974: Add checks for bit lengths
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1122095224
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -167,6 +167,16 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
if (nBits == 0) return // zero length is used for outputValueCalc often.
+ if (primNumeric.width.isDefined) {
+ val width = primNumeric.width.get
+ if (nBits > width)
+ PE(
+ start,
Review Comment:
I tried to test this by using an expression such as `{ 2 * 8 }` but I guess that expression could actually be evaluated at compile time and wasn't exercising the runtime check. I updated the test to get the value from the first byte in the data stream.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #974: Add checks for bit lengths
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1123144789
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -750,21 +751,46 @@ trait ElementBase
// FIXME: bless this method. Deprecate and remove other less reliable things.
final lazy val maybeFixedLengthInBits: MaybeULong = {
- if (isRepresented && repElement.isFixedLength) {
- val bitsMultiplier = repElement.lengthUnits match {
- case LengthUnits.Bits => 1
- case LengthUnits.Bytes => 8
- case LengthUnits.Characters =>
- if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
- }
- if (bitsMultiplier > 0) {
- MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+ val result = {
+ if (isRepresented && repElement.isFixedLength) {
+ val bitsMultiplier = repElement.lengthUnits match {
+ case LengthUnits.Bits => 1
+ case LengthUnits.Bytes => 8
+ case LengthUnits.Characters =>
+ if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
+ }
+ if (bitsMultiplier > 0) {
+ MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+ } else {
+ MaybeULong.Nope
+ }
} else {
MaybeULong.Nope
}
- } else {
- MaybeULong.Nope
}
+
+ // Validate that the number of bits does not exceed the maximum number of
+ // bits allowed for the type
+ if (
+ result.isDefined && repElement.isSimpleType && representation == Representation.Binary
+ ) {
+ primType match {
+ case primNumeric: NodeInfo.PrimType.PrimNumeric =>
+ if (primNumeric.width.isDefined) {
+ val nBits = result.get
+ val width = primNumeric.width.get
+ if (nBits > width) {
+ SDE(
+ "Number of bits %d out of range, must be between 1 and %d bits.",
+ nBits,
+ width,
+ )
Review Comment:
Might be useful to add "binary" and the type in here to make it clear the the type doesn't match the bits. Same suggestion for the other SDE
```suggestion
SDE(
"Number of bits %d out of range for binary %s, must be between 1 and %d bits.",
nBits,
primNumeric.globalQName,
width,
)
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a diff in pull request #974: Add checks for bit lengths
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1123258544
##########
daffodil-codegen-c/src/test/resources/org/apache/daffodil/codegen/c/ex_nums.dfdl.xsd:
##########
@@ -138,7 +138,7 @@
dfdl:byteOrder="littleEndian"/>
<xs:element name="le_int8" type="xs:byte"
dfdl:byteOrder="littleEndian"/>
- <xs:element name="le_int46" type="xs:int"
+ <xs:element name="le_int46" type="xs:integer"
Review Comment:
Nice to see that we can test integer and nonNegativeInteger again in ex_nums again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #974: Add checks for bit lengths
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1123144789
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -750,21 +751,46 @@ trait ElementBase
// FIXME: bless this method. Deprecate and remove other less reliable things.
final lazy val maybeFixedLengthInBits: MaybeULong = {
- if (isRepresented && repElement.isFixedLength) {
- val bitsMultiplier = repElement.lengthUnits match {
- case LengthUnits.Bits => 1
- case LengthUnits.Bytes => 8
- case LengthUnits.Characters =>
- if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
- }
- if (bitsMultiplier > 0) {
- MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+ val result = {
+ if (isRepresented && repElement.isFixedLength) {
+ val bitsMultiplier = repElement.lengthUnits match {
+ case LengthUnits.Bits => 1
+ case LengthUnits.Bytes => 8
+ case LengthUnits.Characters =>
+ if (knownEncodingIsFixedWidth) knownEncodingWidthInBits else -1
+ }
+ if (bitsMultiplier > 0) {
+ MaybeULong(repElement.fixedLengthValue * bitsMultiplier)
+ } else {
+ MaybeULong.Nope
+ }
} else {
MaybeULong.Nope
}
- } else {
- MaybeULong.Nope
}
+
+ // Validate that the number of bits does not exceed the maximum number of
+ // bits allowed for the type
+ if (
+ result.isDefined && repElement.isSimpleType && representation == Representation.Binary
+ ) {
+ primType match {
+ case primNumeric: NodeInfo.PrimType.PrimNumeric =>
+ if (primNumeric.width.isDefined) {
+ val nBits = result.get
+ val width = primNumeric.width.get
+ if (nBits > width) {
+ SDE(
+ "Number of bits %d out of range, must be between 1 and %d bits.",
+ nBits,
+ width,
+ )
Review Comment:
Might be useful to add "binary" and the type in here to make it clear the the type doesn't match the bits.
```suggestion
SDE(
"Number of bits %d out of range for binary %s, must be between 1 and %d bits.",
nBits,
primNumeric.globalQName,
width,
)
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mike-mcgann merged pull request #974: Add checks for bit lengths
Posted by "mike-mcgann (via GitHub)" <gi...@apache.org>.
mike-mcgann merged PR #974:
URL: https://github.com/apache/daffodil/pull/974
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #974: Add checks for bit lengths
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1122110345
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -167,6 +167,16 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
if (nBits == 0) return // zero length is used for outputValueCalc often.
+ if (primNumeric.width.isDefined) {
+ val width = primNumeric.width.get
+ if (nBits > width)
+ PE(
+ start,
Review Comment:
Yup. Poor-man's constant folding using an obvious hack - infoset to be a fake infoset that just throws if you try to access any method on it. Eval expression. No infoset access means it can compute the value. If it touches the infoset, it throws, and determines therefore that it can't constant-fold the expression.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #974: Add checks for bit lengths
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #974:
URL: https://github.com/apache/daffodil/pull/974#discussion_r1120759331
##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/BinaryNumberParsers.scala:
##########
@@ -167,6 +167,16 @@ abstract class BinaryIntegerBaseParser(
def parse(start: PState): Unit = {
val nBits = getBitLength(start)
if (nBits == 0) return // zero length is used for outputValueCalc often.
+ if (primNumeric.width.isDefined) {
+ val width = primNumeric.width.get
+ if (nBits > width)
+ PE(
+ start,
Review Comment:
Definitely need a test to exercise this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org