Closed Bug 1287367 Opened 3 years ago Closed 3 years ago

StreamingLexer should make lexers aware when the input data is truncated

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 3 obsolete files)

My recent refactoring of the Decoder code has made one thing very clear to me: one of the biggest remaining footguns in the shared Decoder code is the way we recover when the input data is truncated. There are a plethora of related virtual methods (FinishInternal(), BeforeFinishInternal(), FinishWithErrorInternal()), some decoders actually depend on the recovery code to produce final output (i.e., from the perspective of the Decoder class, the data is *always* truncated for these decoders), and every decoder seems to use the various methods and flags differently. It's really a mess.

Considering how we might solve this problem, it became clear to me that the real issue is that the decoders' state machines have no way of being notified that the input is truncated. It's pretty easy to provide that feature, though, so let's go ahead and do it.
Here's the meat of the patch series. It's pretty simple: users of StreamingLexer
pass an additional transition to the StreamingLexer constructor which specifies
a "truncated data state". If StreamingLexer detects that the data is truncated,
it invokes that state. The lexer code can then return TerminalState::SUCCESS or
TerminalState::FAILURE, depending on whether or not it was able to salvage the
partially-decoded image.

Not too complicated, but it should allow us to remove a lot of complexity from
the shared Decoder code.

By the way, I promise this is my last new StreamingLexer feature for the
foreseeable future. =) Thanks for reviewing all these huge patches!
Attachment #8771866 - Flags: review?(n.nethercote)
This patch just updates the existing tests to compile. Unlike the real decoders,
which provide Transition::TerminateSuccess() for their truncation states right
now (that's just to emulate the current behavior), the lexers in the tests all
pass Transition::TerminateFailure(), because if an unexpected truncation
happens, we want the test to fail.

This actually found a real bug! There was a test which returned
TerminalState::SUCCESS because of truncation, not because the actual lexer state
machine returned that value. Discovering that issue set me on the path to the
bug fix I posted over in bug 1287246, so this has been very helpful.
Attachment #8771868 - Flags: review?(n.nethercote)
There were some existing truncation-related tests. (Specifically, the ones that
checked for the interaction between StreamingLexer and SourceBuffer::Complete(),
which is called when we know that we aren't going to get any more data.) The
existing tests don't really make sense, and they assume the old behavior of
defaulting to returning TerminalState::SUCCESS on truncation. I've ripped them
out and replaced them with new tests that cover the new code better.
Attachment #8771869 - Flags: review?(n.nethercote)
Comment on attachment 8771868 [details] [diff] [review]
(Part 1b) - Update tests to compile.

Review of attachment 8771868 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to have a comment explaining why Transition::TerminateFailure() is a good choice. But I'm not sure where to put it, because you don't want to repeat it every time. Maybe in an intermediate variable?
Attachment #8771868 - Flags: review?(n.nethercote) → review+
Comment on attachment 8771869 [details] [diff] [review]
(Part 1c) - Replace existing truncation tests with ones that pass and are more thorough.

Review of attachment 8771869 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/TestStreamingLexer.cpp
@@ +914,5 @@
> +}
> +
> +TEST_F(ImageStreamingLexer, SourceBufferTruncatedStateReturningSuccess)
> +{
> +  // Test that a truncated state that returns TerminalState::SUCCESS works.

Do you really mean TerminalState::SUCCESS in this comment?

@@ +923,5 @@
> +}
> +
> +TEST_F(ImageStreamingLexer, SourceBufferTruncatedStateReturningFailure)
> +{
> +  // Test that a truncated state that returns TerminalState::SUCCESS works.

Ditto. Also, the comment should say FAILURE, not SUCCESS.
Attachment #8771869 - Flags: review?(n.nethercote) → review+
Comment on attachment 8771866 [details] [diff] [review]
(Part 1a) - Allow users of StreamingLexer to detect and handle truncation.

Review of attachment 8771866 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: image/StreamingLexer.h
@@ +300,5 @@
> + * this happens by invoking the truncated data state you passed to the
> + * constructor. At this point you can attempt to recover and return
> + * TerminalState::SUCCESS or TerminalState::FAILURE, depending on whether you
> + * were successful. Note that you can't return anything other than a terminal
> + * state in this situation, since there's no more data to read. For the same

Please explain what happens if you do return a non-terminal state. (AFAICT, it's an assertion in assertion-enabled builds and an immediate decoding failure otherwise.)

@@ +302,5 @@
> + * TerminalState::SUCCESS or TerminalState::FAILURE, depending on whether you
> + * were successful. Note that you can't return anything other than a terminal
> + * state in this situation, since there's no more data to read. For the same
> + * reason, your truncated data state shouldn't require any data. (That is, the
> + * @aSize argument you pass to Transition::To() must be zero.)

Please explain what happens if the aSize argument is zero.

@@ +384,5 @@
> +      // there is no more data to receive. That means that yielding or an
> +      // unbuffered read would not make sense, and that the state must require
> +      // zero bytes.
> +      MOZ_ASSERT_UNREACHABLE("Truncated state makes no sense");
> +      return;

I guess it's safe to return early here because mTransition is initialized to TerminalState::FAILURE, and so the image decoding will just immediately fail?

@@ +658,5 @@
> +    // state we should end up in.
> +    LexerTransition<State> transition
> +      = mTruncatedTransition.NextStateIsTerminal()
> +      ? mTruncatedTransition
> +      : aFunc(mTruncatedTransition.NextState(), nullptr, 0);

I'm allergic to putting the '=' on the next line and would do this:

> LexerTransition<State> transition =
>   mTruncatedTransition.NextStateIsTerminal()
>   ? mTruncatedTransition
>   : aFunc(mTruncatedTransition.NextState(), nullptr, 0);

but YMMV.
Attachment #8771866 - Flags: review?(n.nethercote) → review+
Thanks for the quick review!

(In reply to Nicholas Nethercote [:njn] from comment #7)
> @@ +384,5 @@
> > +      // there is no more data to receive. That means that yielding or an
> > +      // unbuffered read would not make sense, and that the state must require
> > +      // zero bytes.
> > +      MOZ_ASSERT_UNREACHABLE("Truncated state makes no sense");
> > +      return;
> 
> I guess it's safe to return early here because mTransition is initialized to
> TerminalState::FAILURE, and so the image decoding will just immediately fail?

Yep, exactly.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> ::: image/test/gtest/TestStreamingLexer.cpp
> @@ +914,5 @@
> > +}
> > +
> > +TEST_F(ImageStreamingLexer, SourceBufferTruncatedStateReturningSuccess)
> > +{
> > +  // Test that a truncated state that returns TerminalState::SUCCESS works.
> 
> Do you really mean TerminalState::SUCCESS in this comment?

I do, but it's worded badly. The idea is that TRUNCATED_SUCCESS returns TerminalState::SUCCESS, and that's really what we're checking for. I've added a more detailed explanation.

The other one you pointed out is just wrong, of course. =) I've fixed it.
This updated version of the patch addresses the review comments and folds
together the previous three patches so they can be landed.
Attachment #8771866 - Attachment is obsolete: true
Attachment #8771868 - Attachment is obsolete: true
Attachment #8771869 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b88cb3db76
Allow users of StreamingLexer to detect and handle truncation. r=njn
https://hg.mozilla.org/mozilla-central/rev/d9b88cb3db76
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.