1223160 - Create a SDP file reader for fuzzing

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

It would speed up our SDP parser fuzzing efforts a lot if we could pass the fuzzed SDP directly via a file into the SDP rather and avoid the overhead of having to start Firefox.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

Bug 1223160: added SDP parser file reader. r?bwc

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8685111 [details]
MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

https://reviewboard.mozilla.org/r/24733/#review22265

::: media/webrtc/signaling/test/sdp_file_parser.cpp:23
(Diff revision 1)
> +#include "signaling/src/sdp/SipccSdpParser.h"
> +
> +extern "C" {
> +#include "signaling/src/sdp/sipcc/sdp.h"
> +#include "signaling/src/sdp/sipcc/sdp_private.h"
> +}

Some of our parse code lives outside of the old sipcc code, and as time passes this will become more and more true. I think we should be using SipccSdpParser here, for this reason.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:40
(Diff revision 1)
> +    SdpTest() : sdp_ptr_(nullptr) {
> +    }

Generally we're supposed to be putting brackets for functions on their own line.

Comment 3

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/24733/#review22265

> Some of our parse code lives outside of the old sipcc code, and as time passes this will become more and more true. I think we should be using SipccSdpParser here, for this reason.

Just to be sure: are you saying that I should use SipccSdpParser.Parse() for this test?

FYI I'm also thinking if we could do a similar implementation on the JSEP level.

Comment 4

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/24733/#review22265

> Just to be sure: are you saying that I should use SipccSdpParser.Parse() for this test?
> 
> FYI I'm also thinking if we could do a similar implementation on the JSEP level.

Yeah. A JSEP fuzzer is probably not a bad idea either.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8685111 [details]
MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

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

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8685111 [details]
MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

https://reviewboard.mozilla.org/r/24733/#review22311

Just a bunch of nits, otherwise looks good.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:7
(Diff revision 2)
> +#include "timecard.h"
> +
> +#include "CSFLog.h"

Can we get away with not including these?

::: media/webrtc/signaling/test/sdp_file_parser.cpp:12
(Diff revision 2)
> +#include <sstream>

I don't see us using stringstream anywhere in here. Is some other header not including something it needs to?

::: media/webrtc/signaling/test/sdp_file_parser.cpp:20
(Diff revision 2)
> +#include "FakeMediaStreamsImpl.h"

Let's add a comment that this needs to be here or the link fails. So, so sad...

::: media/webrtc/signaling/test/sdp_file_parser.cpp:24
(Diff revision 2)
> +using namespace mozilla;
> +
> +const std::string kDefaultFilename((char *)"/tmp/sdp.bin");
> +
> +std::string filename(kDefaultFilename);
> +
> +namespace test {

It isn't actually necessary to put this stuff in the test namespace; the mozilla namespace works fine (see jsep_session_unittest.cpp).

::: media/webrtc/signaling/test/sdp_file_parser.cpp:32
(Diff revision 2)
> +class NewSdpTest : public ::testing::Test {

Let's call this SdpParseTest.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:37
(Diff revision 2)
> +      mSdp = mozilla::Move(mParser.Parse(sdp));

The mozilla::Move shouldn't be necessary.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:44
(Diff revision 2)
> +TEST_F(NewSdpTest, parseSdpFromFile) {
> +  std::streampos size;
> +  char *memblock;

There's no need to declare variables at the beginning of the function, since this is c++ code. Also, make sure to put opening brace on its own line here.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:48
(Diff revision 2)
> +  std::ifstream file(filename.c_str(), std::ios::in|std::ios::binary|std::ios::ate);

Fold to 80

::: media/webrtc/signaling/test/sdp_file_parser.cpp:49
(Diff revision 2)
> +  if (file.is_open()) {

It probably makes sense to ASSERT_TRUE(file.is_open()), so we don't end up with silent failures if we ever put this in automation or something.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:51
(Diff revision 2)
> +    size_t nsize = size;
> +    nsize+=1;
> +    memblock = new char [nsize];
> +    memset(memblock, '\0', nsize);

I would be ok with just using size + 1 instead of having |nsize|, but I'll leave that up to you.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:60
(Diff revision 2)
> +    std::cout << "Parsed SDP" << std::endl;

It probably would make sense to re-serialize what we parsed, just so it is easy to tell that we aren't reading in an empty string or something.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:70
(Diff revision 2)
> +int main(int argc, char **argv) {

Give the bracket its own line.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:73
(Diff revision 2)
> +  if (argc == 2) {
> +    filename = argv[1];
> +  }

It probably wouldn't hurt to error out if argc > 2.

::: media/webrtc/signaling/test/sdp_file_parser.cpp:77
(Diff revision 2)
> +  int result = RUN_ALL_TESTS();
> +
> +  return result;

This could be more terse.

Comment 7

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/24733/#review22311

> I would be ok with just using size + 1 instead of having |nsize|, but I'll leave that up to you.

The reason why I added an extra size_t here is the std::streampos does not allow + operations.

> It probably would make sense to re-serialize what we parsed, just so it is easy to tell that we aren't reading in an empty string or something.

Well the idea here is that our fuzzer will call this executable with all kind of garbled SDP. And the fuzzer nicely records what it feeds into the executable before starting it. I would like to avoid running into bizare serialization issues.

Although it might be interesting to see if we crash in serialization, if the parsing succeeded. Not sure what is the better idea here.

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8685111 [details]
MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

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

Comment 9

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/24733/#review22311

> Well the idea here is that our fuzzer will call this executable with all kind of garbled SDP. And the fuzzer nicely records what it feeds into the executable before starting it. I would like to avoid running into bizare serialization issues.
> 
> Although it might be interesting to see if we crash in serialization, if the parsing succeeded. Not sure what is the better idea here.

It would be pretty hard to crash in serialization, but I guess not impossible. I suppose there are some cases where the re-serialized output could screw up a tty though.

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8685111 [details]
MozReview Request: Bug 1223160: added SDP parser file reader. r+bwc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24733/diff/3-4/

Comment 11

[Nils Ohlmeier [:drno]]

Check back on the try build results...

Comment 12

[Nils Ohlmeier [:drno]]

Try run looks good.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a599f84c2552

Comment 14

[Phil Ringnalda (:philor)]

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