Error recovery

[dguerizec]

Added the ability to force decoding if an index is unknown in the dynamic table.

This is enabled with the flag HPACK_CFG_DEGRADED passed to hpack_decoder().

[dridi]

Bonjour @dguerizec,

This pull request has been a long time coming, far too long. Apologies for taking so much time to come back to you. When I first tried to look at this there were too many things to deal with and it eventually got overtaken by events. Since you had working code, I believe you were able to deal with your use case and I thank you again for making the effort to share your work with me.

Here is a list of things that needed to be addressed, and this is not meant to put the blame on you:

• API breakage with the new flags argument (I know I suggested this one to you)
• No test coverage at all
• New events not documented in the state machines (but otherwise documented)
• Unreliable hpack_unknown_* constants
  • needing strcmp() opens the (unlikely) possibility for false positives
• degraded mode hooked in too many places (my gut feeling then)
• code style violations all over the place (not a big problem)

If anything, all of the above is my responsibility, and I finally took a stab at it.

Revisiting your contribution helped me spot a few holes addressed in:

• 6c9ea80 Propagate dynamic indices
• db7605b Always provide a string length from indexed fields
• 3f80e2b Only compute the length of non-indexed strings

Instead of visible flags, I decided to introduce a whole new concept for what you were doing, and created a new hpack_monitor() function that creates a decoder in "degraded" mode and minimal means to test a monitor:

• 2360565 Introduce an HPACK monitor
• 143cd41 Rudimentary test fixtures for HPACK monitors

Now regarding the features you were interested in, I implemented two of them:

• 9d1b217 Expose header fields addresses to decoders
• 2f0ad21 Tolerate unknown dynamic entries with monitors

One problem I found with your PTR event is that it could trigger multiple times if a field definition was split and resumed in a CONTINUATION frame, the second event bringing an inaccurate address. I decided to expose the address in the FIELD event that was already guaranteed to trigger exactly once, and suffers the same split limitation, however less inaccurate.

Bonus point, no need to touch the state machines.

Regarding the degraded mode, my approach was to handle it from a central place, and add as little workarounds as possible:

• one to skip the validation of an unknown name
• one to give direct access to hpack_unknown_* for better error handling

The validation problem was irrelevant with your patch series because "unknown_name" was fine, but I decided to go with "<unknown name>" to match the "<too big>" precedent for consistency.

The direct access to unknown symbols enables reliable error handling:

if (buf == hpack_unknown_name) {
        /* ... */
}

I didn't implement an equivalent to your RECERR event because by construction it gives you a pointer right after the offending payload in your HPACK input. There's nothing preventing me from adding such an event, but I'd probably add a monitor function to deal with that case instead:

if (buf == hpack_unknown_name) {
        idx = hpack_unknown(hp, &ptr);
        /* ... */
}

I could most likely get an accurate pointer to the beginning of the offending payload (or beginning of the current partial block) just like I did for FIELD events. Another bonus point: I would be able to preserve the current decoding state machine and keep it identical between regular decoders and monitors, offering another monitor-specific facility.

I would be very interested in your opinion on the matter and I would like to thank you again for your contribution. I hope you will accept my apologies for taking so much time to dive into this change.

And also out of curiosity, are you still using your fork of cashpack?