I think there is an issue here.
isBitLengthOk is defined as
val isLimitOk: Boolean = dis.withBitLengthLimit(nBits) {
eParser.parse1(pState)
}
Assert.invariant(isLimitOk)
You turned this invariant check into a PE if !isLimitOk.
I'm going to suggest try not doing that. Just remove the assert.invariant
check. (and the whole isLimitOk variable)
If eParser.parse1(pState) fails, because some other parser calls
PENotEnoughBits, then that
error gets propagated by the next piece of code, which tests for success and
returns on failure.
if (pState.processorStatus ne Success) return
That's what's going to ultimately cause the parse error to be noticed, cause
backtrack, etc.
The dis.withBitLengthLimit only returns false if dis.setBitLengthLimit0b
returns false, and that only happens if the dis.setBitLengthLimit0b call
"doesn't do anything", as in doesn't affect any change on the bit length limit.
It's not quite that because it will return true if you set the bit length limit
to the same value it already has, which is also doing nothing. This boolean
result being false, means we're imposing a length limit, but there is already a
length limit more restrictive.
To the extent this is a length limit, not a length, I think it's not an error
for this to happen.
An example: the test case provides 8 bytes of data. The schema has an element
of type string with explicit length which is computed to be 9 bytes of data.
Setting the limit to 72 bits, when only 64 bits are available may not be an
error, or rather may not be the right error to get first. The first characters
of the string may fail to decode properly triggering a parse error about that,
long before we get to where we're actually accessing and hitting the end of the
64 bits.
So I think a parse error just because dis.withBitLengthLimit returned false is
likely not correct.
________________________________
From: Steve Lawrence <slawre...@apache.org>
Sent: Wednesday, September 23, 2020 7:39 AM
To: dev@daffodil.apache.org <dev@daffodil.apache.org>
Subject: Re: Removing Length Check for Complex Types
I'd be interested to hear others thoughts, but this sounds like the
right behavior to me.
The use of isDefinedForLength for complex types is flawed--it is very
reasonable for complex types to be too large to cache so we cannot call
it to immediately determine if enough data exists for a complex type.
Simple types (that aren't blobs) should never be as big as our cache, so
it is reasonable to maintain the check in that case.
Removing this will break some assumptions, such as isBitLimitOk always
being true. It's possible there are others that some of these failing
test will tease out.
And it will also change the parse behavior which could cause some tests
to run differently. Rather than immediately failing when there is not
enough data for a complex type, it instead will only fail after parsing
the children of the complex type and trying to skip any remaining bits.
This means that the children of specified complex types will always be
parsed, whereas in the past they would not be parsed if there was not
enough data. If a test is specifically testing not enough data for
complex types, the test should be fixed so that those child elements
succeed. There may be other tweaks needed to tests depending on what
tests are trying to accomplish.
On 9/22/20 7:12 PM, Carlson, Ian wrote:
> While pursuing DAFFODIL-2395
> <https://issues.apache.org/jira/browse/DAFFODIL-2395>, I experimented with
> SpecifiedLengthParsers.scala, specifically the check isDefinedForLength. I
> added
> logic to only do this check for non-complex types so we wouldn’t look ahead
> across types that might be so large that it breaks our backtracking.
>
> This immediately caused problems with the Assert.invariant(isLimitOk). It
> appears that isLimitOk can now be false when the parser backtracks on points
> of
> uncertainty. As an attempted fix, I replaced the assert with a processing
> error
> for complexTypes.
>
> This causes some 50 tests to fail with new parse error messages, presumably
> because we aren’t backtracking in the same way as before.
>
> This seems like it might require some additional thought, so I wanted to get
> more eyes on the problem. Does the above solution make sense, or do these
> results spark any new opinions?
>
> A picture containing object, clock Description automatically generatedIan
> Carlson | Software Engineer
>
> Owl Cyber Defense
>
> W_icarlson@_owlcyberdefense.com <https://owlcyberdefense.com/>
>
> Connect with us!
>
> Facebook <https://www.facebook.com/owlcyberdefense/> | LinkedIn
> <https://www.linkedin.com/company/owlcyberdefense/>| Twitter
> <https://twitter.com/owlcyberdefense>
>
> Find us at our next event. Click Here.
> <https://owlcyberdefense.com/resources/events/?utm_source=owl-cyber-defense&utm_medium=email&utm_content=banner&utm_campaign=2020-events>
>
> The information contained in this transmission is for the personal and
> confidential use of the individual or entity to which it is addressed.
>
> If the reader is not the intended recipient, you are hereby notified that any
> review, dissemination, or copying of this communication is strictly
> prohibited.
>
> If you have received this transmission in error, please notify the sender
> immediately*__*
>
