Thanks for your comments, Ralph and Jean-Marc. Please find attached the amended patch: - decodes a sequence of input packets rather than just one (I'm planning on using the Opus test vectors as the seed corpus) - decides on decoder setup and FEC independently of the packet data - uses Opus functions to parse ToC Cheers, Felicia On Sun, Jan 29, 2017 at 9:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Felicia, > > Here's a few comments/questions on your patch: > > > static void ParseToc(const uint8_t toc, TocInfo *const info) { > > Any particular reason you don't use the Opus functions for parsing the > ToC? It seems like opus_packet_get_nb_samples(), > opus_packet_get_bandwidth(), and opus_packet_get_nb_channels() should do > the trick. > > > int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) > > How is that function supposed to work? I noticed that it will only > decode a single packet. How does the fuzzer handle testing for bugs that > only happen for a given sequence of input packets? > > Cheers, > > Jean-Marc > > > > > On 27/01/17 02:48 PM, Felicia Lim wrote: > > Hi all, > > > > I'm working on fuzzing Opus with OSS-Fuzz and have started with the > > decoder. Attached is a patch to add the corresponding fuzz target. > > Please let me know if there are any concerns? > > > > Thanks, > > Felicia > > > > > > _______________________________________________ > > opus mailing list > > opus at xiph.org > > http://lists.xiph.org/mailman/listinfo/opus > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20170210/8570012c/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Add-decoder-fuzz-target.patch Type: text/x-patch Size: 5036 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20170210/8570012c/attachment.bin>
Hi Felicia, Overall the patch looks good to me and it's a pretty reasonable starting points. Some minor comments below. On 10/02/17 02:44 PM, Felicia Lim wrote:> - decodes a sequence of input packets rather than just one (I'm planning > on using the Opus test vectors as the seed corpus)I remember from experimenting with AFL that it didn't like the test vectors because they were very long and caused the fuzzer to run slowly. Depending on the fuzzer you're using, you might want to use shorter vectors. In general, it appears unlikely that any bug would require more than around 5 packets to trigger it.> - decides on decoder setup and FEC independently of the packet dataI'm a little concerned with that one because it means you can never test the case where there's resampling in the first frame. For example, you cannot have a 16000 decoder that starts with a narrowband packet. That being said, I consider those minor issues and I don't see a problem with addressing them in a separate patch if it means we can start fuzzing earlier. Cheers, Jean-Marc> Cheers, > Felicia > > On Sun, Jan 29, 2017 at 9:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > Hi Felicia, > > Here's a few comments/questions on your patch: > > > static void ParseToc(const uint8_t toc, TocInfo *const info) { > > Any particular reason you don't use the Opus functions for parsing the > ToC? It seems like opus_packet_get_nb_samples(), > opus_packet_get_bandwidth(), and opus_packet_get_nb_channels() should do > the trick. > > > int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) > > How is that function supposed to work? I noticed that it will only > decode a single packet. How does the fuzzer handle testing for bugs that > only happen for a given sequence of input packets? > > Cheers, > > Jean-Marc > > > > > On 27/01/17 02:48 PM, Felicia Lim wrote: > > Hi all, > > > > I'm working on fuzzing Opus with OSS-Fuzz and have started with the > > decoder. Attached is a patch to add the corresponding fuzz target. > > Please let me know if there are any concerns? > > > > Thanks, > > Felicia > > > > > > _______________________________________________ > > opus mailing list > > opus at xiph.org <mailto:opus at xiph.org> > > http://lists.xiph.org/mailman/listinfo/opus > > >
On Fri, Feb 10, 2017 at 12:57 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Felicia, > > Overall the patch looks good to me and it's a pretty reasonable starting > points. Some minor comments below. > > On 10/02/17 02:44 PM, Felicia Lim wrote: > > - decodes a sequence of input packets rather than just one (I'm planning > > on using the Opus test vectors as the seed corpus) > > I remember from experimenting with AFL that it didn't like the test > vectors because they were very long and caused the fuzzer to run slowly. > Depending on the fuzzer you're using, you might want to use shorter > vectors. In general, it appears unlikely that any bug would require more > than around 5 packets to trigger it. >I didn't see the same problem with libfuzzer, which oss fuzz is using now, but haven't tried with AFL. Perhaps we can start with the test vectors for now and I can add shorter vectors later.> > - decides on decoder setup and FEC independently of the packet data > > I'm a little concerned with that one because it means you can never test > the case where there's resampling in the first frame. For example, you > cannot have a 16000 decoder that starts with a narrowband packet. > > That being said, I consider those minor issues and I don't see a problem > with addressing them in a separate patch if it means we can start > fuzzing earlier. >OK, I'll merge this first, and follow up in a later patch. Thanks for the review. Cheers, Felicia> > Cheers, > > Jean-Marc > > > Cheers, > > Felicia > > > > On Sun, Jan 29, 2017 at 9:48 PM Jean-Marc Valin <jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> wrote: > > > > Hi Felicia, > > > > Here's a few comments/questions on your patch: > > > > > static void ParseToc(const uint8_t toc, TocInfo *const info) { > > > > Any particular reason you don't use the Opus functions for parsing > the > > ToC? It seems like opus_packet_get_nb_samples(), > > opus_packet_get_bandwidth(), and opus_packet_get_nb_channels() > should do > > the trick. > > > > > int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) > > > > How is that function supposed to work? I noticed that it will only > > decode a single packet. How does the fuzzer handle testing for bugs > that > > only happen for a given sequence of input packets? > > > > Cheers, > > > > Jean-Marc > > > > > > > > > > On 27/01/17 02:48 PM, Felicia Lim wrote: > > > Hi all, > > > > > > I'm working on fuzzing Opus with OSS-Fuzz and have started with the > > > decoder. Attached is a patch to add the corresponding fuzz target. > > > Please let me know if there are any concerns? > > > > > > Thanks, > > > Felicia > > > > > > > > > _______________________________________________ > > > opus mailing list > > > opus at xiph.org <mailto:opus at xiph.org> > > > http://lists.xiph.org/mailman/listinfo/opus > > > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20170210/6551f4b4/attachment-0001.html>