fix: The final frame can not be larger than the Frame Length #281
Conversation
The final frame content length is stored in the body header. This means that while it should be related to the frame length in the header, it is not required. This enforces this requirement. As well as adding a test and test vector for a zero length final frame.
Co-Authored-By: Matt Bullock <[email protected]>
Co-Authored-By: Matt Bullock <[email protected]>
|
@mattsb42-aws That is the edge of the edge. |
| * In the case of the final frame, | ||
| * it MUST not be larger than the frame length. | ||
| */ | ||
| needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), 'Final frame length exceeds frame length.') |
There was a problem hiding this comment.
Strictly speaking, this should be two separate checks, no? If something goes terribly wrong on a non-final frame and I get an error that's talking about final frame length, I'm gonna be confused.
There was a problem hiding this comment.
Perhaps. What about the error message Content length exceeds frame length
There was a problem hiding this comment.
How about:
| needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), 'Final frame length exceeds frame length.') | |
| needs(frameLength === contentLength || isFinalFrame, 'Content length does not match frame length.') | |
| needs(!isFinalFrame || frameLength >= contentLength, 'Final frame length exceeds frame length.') |
There was a problem hiding this comment.
Alternate suggestion:
| needs(frameLength === contentLength || (isFinalFrame && frameLength >= contentLength), 'Final frame length exceeds frame length.') | |
| if (isFinalFrame){ | |
| needs(frameLength >= contentLength, 'Final frame length exceeds frame length.') | |
| }else{ | |
| needs(frameLength === contentLength, 'Content length does not match frame length.') | |
| } |
There was a problem hiding this comment.
Talking about this, having the exact values in question in the error message should resolve this.
Both the error message AND the condition should clearly express the intent.
Further, if this error is ever hit, knowing if you are a final frame or not will GREATLY simplify the debugging.
Also, extent the check to the exact content length as long as we are here.
The final frame content length is stored in the body header.
This means that while it should be related to the frame length in the header,
it is not required.
This enforces this requirement.
As well as adding a test and test vector for a zero length final frame.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: