h2spec: The endpoint MUST ignore any flags that is undefined.

[zizhong]

Currently, h2spec reports the issue as follows.

Hypertext Transfer Protocol Version 2 (HTTP/2)
  4. HTTP Frames
    4.1. Frame Format
           [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:12, flags:0x00, stream_id:0)
           [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [recv] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [send] Raw Data (0x000008061600000000)
           [send] Raw Data (0x0000000000000000)
           [recv] Connection closed
      × 2: Sends a frame with undefined flag
        -> The endpoint MUST ignore any flags that is undefined.
           Expected: PING frame
             Actual: Connection closed

With this patch,

    4.1. Frame Format
           [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:12, flags:0x00, stream_id:0)
           [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [recv] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [send] Raw Data (0x000008061600000000)
           [send] Raw Data (0x0000000000000000)
           [recv] PING Frame (length:8, flags:0x01, stream_id:0)
      ✔ 2: Sends a frame with undefined flag

Finished in 0.0529 seconds
1 tests, 1 passed, 0 skipped, 0 failed

[zizhong]

@masaori335 @maskit Would you review this? It's a small fix for one of the issues reported by h2spec.

[maskit]

I think we can simply remove the check and current_hdr.flags should be untouched. Because setting zero should be done on sender side and we have to ignore them even if some undefined flags are set.

[zizhong]

@maskit You're right. Updated.

[masaori335]

[approve ci]

[atsci]

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/343/

[atsci]

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/357/

[atsci]

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/470/

[atsci]

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1931/

[atsci]

AU check failed! https://ci.trafficserver.apache.org/job/autest-github/341/

[atsci]

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2039/

[atsci]

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/603/

[maskit]

+1

[maskit]

http2_frame_flags_are_valid is used only here and in the regression test, so I think we can remove the function itself, and the condition used in it should be moved into the regression test.

[zizhong]

@maskit fixed.

[maskit]

Thanks.

[zwoop]

@maskit Do we want this for 7.1.0?

[maskit]

@zwoop I think it's nice to have. It matters only if some clients violate the protocol.

[zwoop]

This doesn't cherry-pick cleanly to 7.1.x branch. If we want this back ported, I think someone will have to make a separate PR. I think it's because of some of the clang-tidy that's been run on master.

[zizhong]

@zwoop okay, I'll create one.