Skip to content

Add checks for bit lengths#974

Merged
mike-mcgann merged 1 commit intoapache:mainfrom
mike-mcgann:daffodil-1704-validate-bit-len
Mar 2, 2023
Merged

Add checks for bit lengths#974
mike-mcgann merged 1 commit intoapache:mainfrom
mike-mcgann:daffodil-1704-validate-bit-len

Conversation

@mike-mcgann
Copy link
Member

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

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test of the runtime check.

val width = primNumeric.width.get
if (nBits > width)
PE(
start,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely need a test to exercise this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, minor suggestion for maybe a slightly more helpful error message

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
@mike-mcgann mike-mcgann force-pushed the daffodil-1704-validate-bit-len branch from 593d8f6 to 619adad Compare March 2, 2023 14:43
Copy link
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

<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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see that we can test integer and nonNegativeInteger again in ex_nums again.

@mike-mcgann mike-mcgann merged commit d5c8848 into apache:main Mar 2, 2023
@mike-mcgann mike-mcgann deleted the daffodil-1704-validate-bit-len branch March 2, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants