The Faulty IPC fuzzer supports fuzzing IPDL messages, but quite a bit of data shipped through the IPC layer is sitting inside shared memory and so not actually fuzzed by it. We should extend the Faulty tool to be able to detect Shmem related messages, and fuzz the actual Shmem content.

Haik is looking into this.

Gian-Carlo pointed me at IPC::Channel::ChannelImpl::ProcessOutgoingMessages and It looks like a good place to instrument the code to fuzz shared memory messages would be there in ProcessOutgoingMessages. File descriptors for shared memory regions are written into messages there and at that point we could add/remove/corrupt file descriptors or use a descriptor to access the shared region and alter it. That said I'm having trouble finding a way to exercise shared memory messages in nightly builds. Services that appear to use shared memory messages are $ for f in `hg locate *.ipdl` ; do if grep -i "shmem" $f &>/dev/null; then echo $f ; fi ; done dom/ipc/PContent.ipdl dom/media/gmp/PGMPVideoDecoder.ipdl dom/media/gmp/PGMPVideoEncoder.ipdl dom/media/systemservices/PCameras.ipdl dom/plugins/ipc/PPluginInstance.ipdl ipc/ipdl/test/cxx/PTestDataStructures.ipdl ipc/ipdl/test/cxx/PTestShmem.ipdl ipc/ipdl/test/cxx/PTestSysVShmem.ipdl ipc/ipdl/test/ipdl/error/shmem.ipdl ipc/ipdl/test/ipdl/error/shmem_access_union.ipdl ipc/ipdl/test/ipdl/ok/shmem.ipdl

The locations you mentioned in your first paragraph are also the locations were Faulty is randomly closing pipes.

(In reply to Haik Aftandilian [:haik] from comment #2) > That said I'm having trouble finding a way to exercise shared memory > messages in nightly builds. Mochitests or other unit tests for dom/media should exercise the GMP infrastructure and the Cameras IPC code, so that would be a good start. You can do an entire mochitest run and have it dump logs or crash when Shmem gets used, so you can spot the relevant test. > dom/ipc/PContent.ipdl That one covers pretty much the entire content process, you'll have to dig down to see more details.

Thanks. That led me to the test_gmp_playback.html mochitest which I'm running with "./mach mochitest --e10s dom/media/test/test_gmp_playback.html" which exercises shared memory code. That causes ShadowLayerForwarder::AllocUnsafeShmem() to execute which leads to the child allocating a shared memory segment and sending the size/handle over to the parent process. My plan for this bug is now to 1) Add code to Christoph's fuzzing patch on 777067 to fuzz the ShmemCreated message sent to the parent process 2) To fuzz the contents of the shared memory segment before and/or after sending the ShmemCreated message 3) And to randomly fuzz the contents of any IPDL shared memory segments that have already been shared to the parent. The shared segments are saved in a map in IPDL-generated P* classes so we could access them from there somehow and corrupt them.

Haik, are you actually working on this? I think freddy was starting to looking at picking this up since i assumed you were busy with other things.

(In reply to Paul Theriault [:pauljt] from comment #6) > Haik, are you actually working on this? I think freddy was starting to > looking at picking this up since i assumed you were busy with other things. I was planning to work on it this quarter, but got sidetracked by some printing issues holding up the content sandbox and haven't got to it. That would be great if freddy wants to look at this.

Assigned as per previous comment.

I can not work on this after all. Can you, cdiehl?

Probably not this Q but I did ask ckerschb to help out, need to check if and when he gets some spare time.

WIP Example: ./ff-asan-debug/dist/bin/firefox -CreateProfile fuzzing-shmem SHMEM_FUZZER_MUTATION_FACTOR=80000 SHMEM_FUZZER_ENABLE=1 SHMEM_FUZZER_ENABLE_LOGGING=1 ./ff-asan-debug/dist/bin/firefox -P fuzzing-shmem -no-remote The expected output would be: [...] [SharedMemoryFuzzer] shmem of size: 40 / mutations: 1 [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem of size: 4014080 / mutations: 33 [SharedMemoryFuzzer] shmem of size: 4014080 / mutations: 33 [SharedMemoryFuzzer] shmem is of size 0. [SharedMemoryFuzzer] shmem of size: 4014080 / mutations: 6 [SharedMemoryFuzzer] shmem of size: 4014080 / mutations: 12 [SharedMemoryFuzzer] shmem of size: 4014080 / mutations: 22 [...] There are some issues: * I have tested this on MacOS and Ubuntu, it should theoretically work also on Android but not on Windows. * I could not find |mAllocSize| for MacOS, only a Size() but this one is returning |mMappedSize|. * The following assertion fails if SHMEM_FUZZER_MUTATION_FACTOR is set to a too low value (too many mutations). Assertion failure: !strncmp(header->mMagic, sMagic, sizeof(sMagic)) (invalid segment), at /srv/repos/mozilla/mozilla-inbound/ipc/glue/Shmem.cpp:271

Comment on attachment 8870387 [details] [diff] [review] shmem.diff Review of attachment 8870387 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks pretty good. Besides all the nits I have, I have two additional comments: * You are not using ChangeByte, are you planning on doing so? * I am not sure if |virtual void* memory() const override| is the right entry point, you would have to check with someone who knows how our shared memory implementation works. ::: tools/fuzzing/common/FuzzingMutate.cpp @@ +11,5 @@ > +namespace fuzzing { > + > +/* static */ > +void > +FuzzingMutate::ChangeBit(uint8_t *data, size_t length) nit: pointer goes with type, and please prefix arguments. @@ +14,5 @@ > +void > +FuzzingMutate::ChangeBit(uint8_t *data, size_t length) > +{ > + size_t offset = RandomIntegerRange<size_t>(0, length); > + data[offset] ^= (1 << RandomIntegerRange<unsigned char>(0, 9)); this method definitely lacks a comment, please describe what you are doing. @@ +19,5 @@ > +} > + > +/* static */ > +void > +FuzzingMutate::ChangeByte(uint8_t *data, size_t length) nit: pointer goes with type and prefix args with a @@ +22,5 @@ > +void > +FuzzingMutate::ChangeByte(uint8_t *data, size_t length) > +{ > + size_t offset = RandomIntegerRange<size_t>(0, length); > + data[offset] = RandomInteger<unsigned char>(); please fix indendation and add comment what this byte mutation method is doing in detail ::: tools/fuzzing/common/FuzzingMutate.h @@ +6,5 @@ > + > +#ifndef mozilla_fuzzing_FuzzingMutate_h > +#define mozilla_fuzzing_FuzzingMutate_h > + > +#include "mozilla/Assertions.h" do you need Assertions.h in here? @@ +17,5 @@ > +class FuzzingMutate > +{ > +public: > + static void ChangeBit(uint8_t *data, size_t length); > + static void ChangeByte(uint8_t *data, size_t length); Why can't those two methods not live in FuzzingTraits or in SharedMemoryFuzzer directly? Are they going to be reused by something else eventually? nit: please prefix args with a ::: tools/fuzzing/common/FuzzingTraits.cpp @@ +24,5 @@ > return FuzzingTraits::Random(aProbability) == 0; > } > +/*static */ > +size_t > +FuzzingTraits::Frequency(const size_t size, const uint64_t factor) nit: please prefix arguments with a ::: tools/fuzzing/common/FuzzingTraits.h @@ +89,4 @@ > public: > static unsigned int Random(unsigned int aMax); > static bool Sometimes(unsigned int aProbability); > + static size_t Frequency(const size_t size, const uint64_t factor); nit: please prefix arguments with a ::: tools/fuzzing/shmem/SharedMemoryFuzzer.cpp @@ +6,5 @@ > + > +#include "FuzzingMutate.h" > +#include "FuzzingTraits.h" > +#include "nsDebug.h" > +#include "prenv.h" not sure if you need to include nsDebug and prenv - please check. @@ +33,5 @@ > + sIsLoggingEnabled = !!PR_GetEnv("SHMEM_FUZZER_ENABLE_LOGGING"); > + sInitialized = true; > + } > + > + return sIsLoggingEnabled; nit: no empty line before return @@ +64,5 @@ > + return sPropValue; > + } > + } > + > + return sPropValue; nit: no empty line before return @@ +70,5 @@ > + > +/* static */ > +uint64_t > +SharedMemoryFuzzer::MutationFactor() > +{ nit: other functions in this file only use 2 space indendation, please be consistent. @@ +88,5 @@ > + return sPropValue; > + } > + } > + > + return sPropValue; nit: no empty line before return @@ +92,5 @@ > + return sPropValue; > +} > + > +/* static */ > +void * nit: pointer goes with type, so it's void* @@ +93,5 @@ > +} > + > +/* static */ > +void * > +SharedMemoryFuzzer::MutateSharedMemory(void *memory, size_t size) nit: please prefix arguments with a @@ +114,5 @@ > + > + // The likelihood when a value gets fuzzed of this object. > + if (!FuzzingTraits::Sometimes(MutationProbability())) { > + return memory; > + } I think all of the above if statements can be bundled together into one: if (!IsEnabled() || size == 0 || !memory || ...) { ::: tools/fuzzing/shmem/SharedMemoryFuzzer.h @@ +15,5 @@ > +SHMEM_FUZZER_ENABLE=1 > +SHMEM_FUZZER_ENABLE_LOGGING=1 (optional) > +SHMEM_FUZZER_MUTATION_PROBABILITY=2 (optional) > +SHMEM_FUZZER_MUTATION_FACTOR=500 (optional) > +*/ Nit: please use leading * when adding a block comment.

Updated the patch addressing the nits above. (In reply to Christoph Kerschbaumer [:ckerschb] from comment #12) > Comment on attachment 8870387 [details] [diff] [review] > shmem.diff > * You are not using ChangeByte, are you planning on doing so? Yes, after further review comments. > * I am not sure if |virtual void* memory() const override| is the right > entry point, you would have to check with someone who knows how our shared > memory implementation works. billm suggested this entry point. > Why can't those two methods not live in FuzzingTraits or in > SharedMemoryFuzzer directly? Are they going to be reused by something else > eventually? FuzzingTraits consist of helper functions, mostly random related. I would like to separate those from functions which mutate/generate content. Also I want to have the possibility in adding new ones to FuzzingMutate to reuse them. This will happen when the IPC fuzzer re-organized in our tree. > ::: tools/fuzzing/shmem/SharedMemoryFuzzer.cpp > > +#include "FuzzingMutate.h" > > +#include "FuzzingTraits.h" > > +#include "nsDebug.h" > > +#include "prenv.h" > > not sure if you need to include nsDebug and prenv - please check. prenv.h is required for PR_GetEnv() nsDebug.h is required for printf_stderr() > I think all of the above if statements can be bundled together into one: > if (!IsEnabled() || size == 0 || !memory || ...) { I can do that after further review comments, this is currently for debugging purposes.

Billm, any updates from your side?

Sorry, I'll get to it tomorrow.

Comment on attachment 8870550 [details] [diff] [review] shmem.v1.1.diff Review of attachment 8870550 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/SharedMemoryBasic_android.h @@ +34,5 @@ > virtual void* memory() const override > { > +#ifdef FUZZING > + return SharedMemoryFuzzer::MutateSharedMemory(mMemory, mAllocSize); > +#endif It would be good to put the other return statement in an #else block. Otherwise I worry that the compiler might complain about unreachable code. Same for the other SharedMemoryBasic implementations. Also, you need to #include "SharedMemoryFuzzer.h" in this file. ::: tools/fuzzing/common/FuzzingMutate.cpp @@ +9,5 @@ > + > +namespace mozilla { > +namespace fuzzing { > + > +/* static */ static should go after the comment, not before. @@ +17,5 @@ > +void > +FuzzingMutate::ChangeBit(uint8_t* aData, size_t aLength) > +{ > + size_t offset = RandomIntegerRange<size_t>(0, aLength); > + aData[offset] ^= (1 << RandomIntegerRange<unsigned char>(0, 9)); Why 9? 1<<8 is 512, which is too big to fit in a byte. ::: tools/fuzzing/common/FuzzingTraits.h @@ +89,4 @@ > public: > static unsigned int Random(unsigned int aMax); > static bool Sometimes(unsigned int aProbability); > + static size_t Frequency(const size_t aSize, const uint64_t aFactor); I don't understand what this function is supposed to do. It should have a comment. ::: tools/fuzzing/shmem/SharedMemoryFuzzer.cpp @@ +11,5 @@ > +#include "SharedMemoryFuzzer.h" > + > +#define SHMEM_FUZZER_DEFAULT_MUTATION_PROBABILITY 2 > +#define SHMEM_FUZZER_DEFAULT_MUTATION_FACTOR 500 > +#define SHMEM_FUZZER_LOG(fmt, args...) \ It would be better to use MOZ_LOG, but I guess I don't care that much. @@ +39,5 @@ > +/* static */ > +bool > +SharedMemoryFuzzer::IsEnabled() > +{ > + return !!PR_GetEnv("SHMEM_FUZZER_ENABLE"); Why don't you cache this one the same way you cache the other environment vars? ::: tools/fuzzing/shmem/SharedMemoryFuzzer.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_dom_SharedMemoryFuzzer_h__ Please remove the two underscores at the end here. ::: tools/fuzzing/shmem/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +SOURCES += [ Please use UNIFIED_SOURCES. @@ +8,5 @@ > + 'SharedMemoryFuzzer.cpp' > +] > + > +EXPORTS += [ > + 'SharedMemoryFuzzer.h' Since this file puts its definitions in mozilla::ipc, you should export to mozilla/ipc. Please use EXPORTS.mozilla.ipc instead of EXPORTS.

This update fixes the above issues in comment #16. The patch got review+ before, hence I will carry it over and set checkin-needed. Thanks everybody! :-)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2584d9b5b49c Add fuzzer for SharedMemory. r=billm