1437169 - Improve error checking in parse_fingerprint in attribute_type.rs

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3)

Description

[Dan Minor [:dminor]]

There's minimal error checking in parse_fingerprint, more is needed.

Comment 1

[Dan Minor [:dminor]]

If we don't want to do it in Rust, we could also move the fingerprint error checking in sipcc to a base class and use it in both implementations.

Comment 2

[Johannes Willbold]

Attachment: Bug 1437169: Improved error checking in the the fingerprint parsing,

Improved the error checking in the fingerprint parsing
Changed the way the fingeerprint stores the hash algorihtm to an enum.
Extended the rust unit test for fingerprint
Fixed C++ unit tests

Review commit: https://reviewboard.mozilla.org/r/257634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257634/

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8992795 [details]
Bug 1437169: Improved error checking in the the fingerprint parsing,

https://reviewboard.mozilla.org/r/257634/#review265068

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:592
(Diff revision 1)
> +      default:
> +        algorithm = '?';

I am not a fan of using default when switching over an enum, I prefer that we put every possible value in here, so the compiler will complain if something is added to the enum but not to the switch statement.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:941
(Diff revision 1)
>      }
> +
> +    let fingerprint_token = tokens[1].to_string();
> +    let parse_tokens = |expected_len| -> Result<Vec<u8>, SdpParserInternalError>{
> +        let bytes = fingerprint_token.split(":")
> +                                     .map(|byte_token| u8::from_str_radix(byte_token, 16))

I wonder if we should reject stuff like "0000A" and "B".

I assume this already fails for "" and "0xFB"?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:961
(Diff revision 1)
> +        unknown => {
> +            return Err(SdpParserInternalError::Generic(
> +                format!("fingerprint contains unknown hash algorithm '{}'", unknown)
> +            ))
> +        }

We should ignore (and maybe warn) when we encounter a fingerprint type we do not support.

Comment 4

[Johannes Willbold]

Comment on attachment 8992795 [details]
Bug 1437169: Improved error checking in the the fingerprint parsing,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257634/diff/1-2/

Comment 5

[Johannes Willbold]

Comment on attachment 8992795 [details]
Bug 1437169: Improved error checking in the the fingerprint parsing,

https://reviewboard.mozilla.org/r/257634/#review265068

> I wonder if we should reject stuff like "0000A" and "B".
> 
> I assume this already fails for "" and "0xFB"?

Currenlty, "" and "0xFB" gets rejected but "0000A" and "B" is accepeted. The standard says they should be rejected as well. I added an extra condition that checks that the byte tokens have exaclty 2 hexdigits. Also, I added these special cases to the testcases.

> We should ignore (and maybe warn) when we encounter a fingerprint type we do not support.

Now, it will ignore the fingerpint line and output a warning about it.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa652865d3ad
Improved error checking in the the fingerprint parsing, r=bwc

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/aa652865d3ad