777067 - (fuzzing-ipc-ipdl) [meta] Fuzzing: IPC Protocol Definition Language (IPDL) Protocols

Status: NEW

(Core :: Fuzzing, defect)

Comment 2

[Daniel Veditz [:dveditz]]

Hard to be certain because there was no initial description, but the task described in bug 516716 comment 2--fuzz the IPDL infrastructure--does not seem to be a duplicate of this bug. I hope Sicking speaks up about what he meant, but from the summary I thought the aim of this bug would be to find flaws in the implementations on each side of specific APIs, and not in the generic IPDL mechanism itself (which bug 516716 covers). There's room (and need) for both.

If you duped this after talking with Sicking elsewhere I apologize.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It doesn't really matter to me.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

what Dan said was what was intended.

Comment 5

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 6

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't think we would hold the release if we haven't fuzzed. But we should start fuzzing asap since otherwise there are bound to be exploits in there.

Comment 8

[Christoph Diehl [:posidron]]

In opt/debug builds I am only hitting controlled aborts with DCHECK() for a data type inside a message. In opt/non-debug builds we see warnings but no crashes or aborts.

The fuzzing process got much more stable than it was a few month ago. Haven't found any other abnormal crashes or aborts as in DCHECK() or mozalloc_abort due too to large allocations.

I will move on in my Q4 goal plan and may return at the end of the year again.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is anything happening here? I believe this is one of the weakest point in the FirefoxOS security model, so actually testing it would likely find some of the low-hanging fruit that attackers are likely to find.

Comment 10

[Daniel Veditz [:dveditz]]

Christoph filed 13 bugs of which one was fixed. Hasn't seemed to be much interest in these so we've put the effort into things like Web Audio whose bugs get fixed.

Comment 11

[(no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> Christoph filed 13 bugs of which one was fixed. Hasn't seemed to be much
> interest in these so we've put the effort into things like Web Audio whose
> bugs get fixed.

What's the list of those bugs?  Is there any chance that we focus on IPDL now that Web Audio is in a better shape?

Comment 12

[Christoph Diehl [:posidron]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #11)
> What's the list of those bugs?
https://bugzilla.mozilla.org/showdependencytree.cgi?id=777067&hide_resolved=1

Comment 13

[(no longer active)]

I think the bug list you mentioned is the dependency list for this bug, sorry that I missed that.

I went through those bugs just now, and it's not clear to me how we can reproduce them.  If they are reproducible, we can look into finding people to work on them.  And we're definitely interested in these kinds of bugs in general.  :-)

Comment 14

[Christoph Diehl [:posidron]]

I might have chosen the wrong word for the attachment in each bug, but inside each callstack.txt there is a also a logged GDB session which prints/displays the message content. 

In each message we have changed the value of certain data types to an arbitrary value of that data type by acting as a MITM.

At that time it was possible to reproduce the crashes with the seed value but since then we had many code changes and it is unlikely that those crashes still reproduce with the same seed.

I will look again at it in this quarter and will update the fuzzing patch.

Comment 15

[Christoph Diehl [:posidron]]

The fuzzer has been integrated into their respective forked repositories. The new build instructions are located in the URL field.

https://github.com/posidron/mozilla-central/compare/faulty
https://github.com/posidron/gonk-misc/compare/faulty
https://github.com/posidron/B2G/compare/faulty

Comment 16

[Christoph Diehl [:posidron]]

Update

* logging output is now thread-safe with printf_error()
* added logging functionality of messages in non-debug builds
* added functionality to fuzz either parent or child processes

Big thanks to dhylands for his C++ wisdom!

Comment 17

[Christoph Diehl [:posidron]]

Attachment: faulty.diff

Daniel Veditz and I talked about a possible integration of the fuzzer in our tree - hidden behind a build flag.

Comment 18

[Christoph Diehl [:posidron]]

Attachment: default-gecko-config.diff

Comment 19

[Christoph Diehl [:posidron]]

Attachment: faulty-tool.diff

Comment 20

[Christoph Diehl [:posidron]]

Ben, could you find a person to review those patches?

Comment 21

[Dave Hylands [:dhylands]]

Comment on attachment 8341918 [details] [diff] [review]
faulty-tool.diff

Review of attachment 8341918 [details] [diff] [review]:
-----------------------------------------------------------------

::: faulty.sh
@@ +22,5 @@
> +EOF
> +exit
> +}
> +
> +while getopts "hpdlqwcros:b:" opt

You have an o here, and no o below

@@ +48,5 @@
> +    ./build.sh gecko && ./flash.sh gecko
> +    exit
> +fi
> +
> +adb reboot

Why the reboot?

./flash.sh gecko will stop and restart b2g.

@@ +52,5 @@
> +adb reboot
> +adb wait-for-device
> +adb logcat -c
> +
> +if [[ $FAULTY_PICKLE = 1 ]] ; then

So rather than compare to 1, I probably would say:

if [ -n "$FAULTY_PICKLE" ] ; then

This will compare if the string is non-null (i.e. has length > 1)

Then you can set FAULTY_PICKLE to anything but the empty string and it will get passed along.

@@ +53,5 @@
> +adb wait-for-device
> +adb logcat -c
> +
> +if [[ $FAULTY_PICKLE = 1 ]] ; then
> +    export GDBSERVER_ENV="$GDBSERVER_ENV FAULTY_PICKLE="$FAULTY_PICKLE" "

I'm not sure what you're trying to do with the nested double quotes. Anytime I see stuff like this I usually assume the author doesn't understand bash quoting.

The way bash quoting works, you've created 2 strings: "$GDBSERVER_ENV FAULTY_PICKLE=" and " " and then stuck the expanded $FAULTY_PICKLE in between them. This will produce the exact same thing as:
"$GDBSERVER_ENV FAULTY_PICKLE=$FAULTY_PICKLE " and the second form is cleaner (bash variables are expanded inside double quotes, but not inside single quotes.

If for some perverse reason FAULTY_PICKLE were set to "1 2", then the way you've coded things "aaaa "$FAULTY_PICKE" bbbb" is the same as "aaaa 1" "2 bbbb" which is 2 words.
"aaaa $FAULTY_PICKLE bbbb" is the same as "aaaa 1 2 bbbb" which is one word (it's a subtle but important thing to understand)

You might have also been trying to put the double quotes around "$FAULTY_PICKLE"  but that doesn't work when already inside double quotes (the quote before the " will terminate double quoting introduced after the =, the " after PICKLE will resume double quoting mode.

Personally, I would assume that $FAULTY_PICKLE won't contain any embedded spaces and not bother trying to put the quotes around $FAULTY_PICKLE.

@@ +84,5 @@
> +if [[ $MOZ_DEBUG_CHILD_PROCESS = 1 ]] ; then
> +    export GDBSERVER_ENV="$GDBSERVER_ENV MOZ_DEBUG_CHILD_PROCESS="$MOZ_DEBUG_CHILD_PROCESS" "
> +fi
> +
> +export GDBSERVER_ENV="$GDBSERVER_ENV MOZ_IPC_MESSAGE_LOG=1 MOZ_CRASHREPORTER=1 MOZ_DEBUG_APP_PROCESS=1 NSPR_LOG_MODULES=

You're missing the trailing double quote on this line.

@@ +86,5 @@
> +fi
> +
> +export GDBSERVER_ENV="$GDBSERVER_ENV MOZ_IPC_MESSAGE_LOG=1 MOZ_CRASHREPORTER=1 MOZ_DEBUG_APP_PROCESS=1 NSPR_LOG_MODULES=
> +
> +echo "Set environment variables: "$GDBSERVER_ENV

And I would have put the double quote at the end of the line here.

echo "Set environment variables: $GDBSERVER_ENV"

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 8341916 [details] [diff] [review]
default-gecko-config.diff

Review of attachment 8341916 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me.

Comment 23

[Dave Hylands [:dhylands]]

Comment on attachment 8341915 [details] [diff] [review]
faulty.diff

Review of attachment 8341915 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/pickle.h
@@ +111,1 @@
>      return WriteBytes(&value, sizeof(value));

This probably doesn't really matter, but couldn't this have been simplified by just fuzzing WriteBytes and then you wouldn't need to have all of the fuzzers per custom type?

::: ipc/glue/Faulty.cpp
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace ipc {
> +
> +unsigned int Faulty::defaultProbability = DEFAULT_PROBABILITY;

This seems like a poorly named variable.

I was assuming that a probability would be 0-100 or something. Perhaps just a comment to describe what this really means would help?

@@ +18,5 @@
> +Faulty::Faulty() {
> +    Init();
> +}
> +
> +void Faulty::Init() {

nit: Open curly for functions should be on the next line.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures

@@ +21,5 @@
> +
> +void Faulty::Init() {
> +    FAULTY_LOG("Initializing.");
> +
> +    fuzzPipes = !!PR_GetEnv("FAULTY_PIPE");

This only tests that the environment variable was defined or not. If FAULTY_PIPE=0 were set in the environment, then fuzzPipes would be set to true.

@@ +76,5 @@
> +    return Random(probability) == 0;
> +}
> +
> +const char *Faulty::RandomElement(const char *arr[]) {
> +    return arr[Random(sizeof(arr) / sizeof(arr[0]))];

This doesn't work. sizeof(arr) == sizeof(char *) and sizeof arr[0] is 1

You can use templates with sized arrays (I've seen this somewhere in the codebase, but I can't find it). Bit inside a regular function there is no way to infer the size of the array. You have to pass it as an explicit argument.
See: http://stackoverflow.com/questions/37538/how-do-i-determine-the-size-of-my-array-in-c for a more detailed discussion.
In particular, the second one discusses passing arrays as arguments.

@@ +119,5 @@
> +    }
> +}
> +
> +void Faulty::MutateChar(char *v) {
> +    *v = (char)(Random(127));

Use SCHAR_MAX rather than 127.

You may want to avoid the value zero (or maybe not). It will wind up terminating a string. But maybe thats ok.

This function seems to be called using a string object which means that this may insert a null into the middle of a string, which may cause undesriable results (but I'm not sure)

@@ +133,5 @@
> +    }
> +}
> +
> +void Faulty::MutateUChar(unsigned char *v) {
> +    *v = (unsigned char)(Random(255));

Use UCHAR_MAX instead of 255

@@ +157,5 @@
> +            case 1:
> +                *v = RandomFloatingPointRange<double>(DBL_MIN, DBL_MAX);
> +                break;
> +        }
> +    }

Are you missing a return inside the if (useLargeValues) ?

Without a return, line 162 will always overwrite the results from line 155 or line 158

@@ +182,5 @@
> +            case 1:
> +                *v = RandomFloatingPointRange<float>(FLT_MIN, FLT_MAX);
> +                break;
> +        }
> +    }

Same comment about missing return.

@@ +207,5 @@
> +            case 1:
> +                *v = LargeValue<int16_t>(2.0, 15);
> +                break;
> +        }
> +    }

missing return? This one is a bit more subtle since FuzzIntegralType may leave the value untouched, so the missing return might be deliberate here?

It looks like all of the rest of the Mutate functions have a similar flaw/feature.

@@ +254,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<int>(2.0, 31);

Use sizeof(int) * CHAR_BIT - 1 rather than 31

@@ +279,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<uint32_t>(2.0, 32);

use sizeof(int) * CHAR_BIT rather than 32

@@ +304,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<long>(2.0, 63);

On a 32-bit machine sizeof(long) typically == 4.

Wouldn't it make more sense to use: (sizeof(long) * CHAR_BIT) - 1 rather than 63?

@@ +329,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<unsigned long>(2.0, 64);

Similar for 64

@@ +378,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<uint64_t>(2.0, 127);

Similar comment about sizeof and hard-coded 127

@@ +403,5 @@
> +            case 0:
> +                *v = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *v = LargeValue<int64_t>(2.0, 128);

Similar comment about sizeof and hard-coded 128

::: ipc/glue/Faulty.h
@@ +56,5 @@
> +}
> +
> +template <typename T>
> +void FuzzIntegralType(T* v) {
> +    switch (rand() % 4) {

Your case's should be 0 thru 3, not 1 thru 4

@@ +76,5 @@
> +  }
> +}
> +
> +class Faulty {
> +    private:

nit: Put the public stuff first, and the private stuff after that.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Classes

@@ +91,5 @@
> +        unsigned int Random(unsigned int max);
> +        const char *RandomElement(const char* arr[]);
> +        bool GetChance(unsigned int probability);
> +
> +        bool fuzzPipes;

nit: member variables should start with m
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Prefixes

@@ +97,5 @@
> +        bool useLargeValues;
> +        bool targetChilds;
> +        bool targetParent;
> +
> +        static unsigned int defaultProbability;

nit: statics should be prefixed with an s
(see Prefixes above)

@@ +100,5 @@
> +
> +        static unsigned int defaultProbability;
> +
> +        void MutateBool(bool *value);
> +        void FuzzBool(bool *value, unsigned int probability=defaultProbability);

nit: arguments should be prefixed with an a
(see Prefixes above)

Comment 24

[Christoph Diehl [:posidron]]

Attachment: faulty.diff

Thank you very much for your comments dhylands.

Comment 25

[Christoph Diehl [:posidron]]

Attachment: faulty.sh.diff

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8342308 [details] [diff] [review]
faulty.sh.diff

Review of attachment 8342308 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me.

When making revisions to a patch, it's nice if you keep the description exactly the same, except for adding a v9 (where 9 increments by 1 each time).

This makes doing interdiffs (coming v2 of a patch with v3 of a patch easier).

Comment 27

[Dave Hylands [:dhylands]]

Comment on attachment 8342295 [details] [diff] [review]
faulty.diff

Review of attachment 8342295 [details] [diff] [review]:
-----------------------------------------------------------------

Since I'm not really a peer of this code, I probably shouldn't be the one to give it r+.

So I'll give it a feedback+.

::: ipc/glue/Faulty.cpp
@@ +223,5 @@
> +            case 0:
> +                *aValue = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *aValue = LargeValue<int16_t>(2.0, 15);

nit: Since you fixed the other ones, this should probably be sizeof(int16_t) * CHAR_BITS + 1

@@ +251,5 @@
> +            case 0:
> +                *aValue = limits[rand() % (sizeof(limits) / sizeof(limits[0]))];
> +                break;
> +            case 1:
> +                *aValue = LargeValue<uint16_t>(2.0, 16);

Similarly for here

Comment 28

[Christoph Diehl [:posidron]]

Attachment: faulty_v3.diff

Fixed nits from comment 27.

Comment 29

[Christoph Diehl [:posidron]]

Attachment: faulty_v3.diff

Comment 30

[Dave Hylands [:dhylands]]

Comment on attachment 8343667 [details] [diff] [review]
faulty_v3.diff

Review of attachment 8343667 [details] [diff] [review]:
-----------------------------------------------------------------

As I mentioned previously, since I'm not a peer, I shouldn't really be the one giving the r+.

Assigning the final review to bent.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8343667 [details] [diff] [review]
faulty_v3.diff

Review of attachment 8343667 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good! Thanks for putting this together.

There's enough here that needs to be cleaned up that I'd like to see a second patch so I'm clearing the review flag for now.

::: configure.in
@@ +1577,5 @@
>  dnl ========================================================
> +dnl = Enable Faulty IPC fuzzer
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(ipc-fuzzer,
> +[  --enable-ipc-fuzzer       Enable IPC fuzzer (default=no)],

It looks to me like Faulty is only going to work on linux/mac, right? This should cause a configure error on non-posix builds.

::: ipc/chromium/src/base/pickle.cc
@@ +540,2 @@
>    if (!WriteInt(static_cast<int>(value.size())))
>      return false;

Hm, if MOZ_FAULTY is defined won't this trigger warnings about unreachable code? I think you should #ifdef MOZ_FAULTY/#else/#endif these.

::: ipc/chromium/src/base/pickle.h
@@ +12,5 @@
>  #include "base/string16.h"
>  
> +#ifdef MOZ_FAULTY
> +    #include "mozilla/ipc/Faulty.h"
> +    #include "nsXULAppAPI.h"

Hm, why do you need nsXULAppAPI.h here?

::: ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ +33,5 @@
>  #include "chrome/common/ipc_message_utils.h"
>  #include "mozilla/ipc/ProtocolUtils.h"
>  
> +#ifdef MOZ_FAULTY
> +    #include "mozilla/ipc/Faulty.h"

Nit: Please don't indent includes

::: ipc/glue/Faulty.cpp
@@ +16,5 @@
> +
> +
> +Faulty::Faulty()
> +{
> +    Init();

I think you might as well remove the Init() method and just move the code here since there's no other caller.

@@ +21,5 @@
> +}
> +
> +void Faulty::Init()
> +{
> +    FAULTY_LOG("Initializing.");

Can you make the FAULTY_LOG be dependent on an env variable too? (maybe you could add a FAULTY_LOG_DISABLE one so that the default is to log everything). My reasoning here is that I'd like to build this code normally but I might not always want to see the output.

@@ +22,5 @@
> +
> +void Faulty::Init()
> +{
> +    FAULTY_LOG("Initializing.");
> +	//MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());

Nit: Commented-out code should be removed.

However, this concerns me a little... Can we really not assert which thread the class gets created on?

@@ +24,5 @@
> +{
> +    FAULTY_LOG("Initializing.");
> +	//MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +
> +    mFuzzPipes = !!PR_GetEnv("FAULTY_PIPE");

Nit: It would be nice to document somewhere what all of these env variables mean.

@@ +26,5 @@
> +	//MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +
> +    mFuzzPipes = !!PR_GetEnv("FAULTY_PIPE");
> +    mFuzzPickle = !!PR_GetEnv("FAULTY_PICKLE");
> +    mTargetChilds = !!PR_GetEnv("FAULTY_CHILDS");

Nit: The plural of "child" is "children", so I'd prefer mTargetChildren and FAULT_CHILDREN

@@ +33,5 @@
> +    const char *userSeed = PR_GetEnv("FAULTY_SEED");
> +    const char *userProbability = PR_GetEnv("FAULTY_PROBABILITY");
> +
> +    if (userSeed) {
> +        SetSeed(std::atoi(userSeed));

Hm, I get nervous generally using atoi since its error handling is... crappy... For this use case I guess it doesn't matter too much but strtol could be used much more safely.

@@ +38,5 @@
> +    } else {
> +        SetSeed(static_cast<unsigned int>(std::time(0)));
> +    }
> +    if (userProbability) {
> +        sDefaultProbability = std::atoi(userProbability);

This can't be allowed to be 0...

@@ +48,5 @@
> +    FAULTY_LOG("Strategy: Pipes = %d\n", mFuzzPipes);
> +    FAULTY_LOG("Faulty is initialized for this process.\n");
> +}
> +
> +bool Faulty::IsValidProcessType()

This result of this function won't ever change so why not just set a bool "mIsValidProcessType" in the constructor and remove this function?

@@ +73,5 @@
> +    this->mSeed = aSeed;
> +    srandom(this->mSeed);
> +}
> +
> +unsigned int Faulty::Random(unsigned int aMax)

Please MOZ_ASSERT(aMax > 0) here.

@@ +75,5 @@
> +}
> +
> +unsigned int Faulty::Random(unsigned int aMax)
> +{
> +    return (unsigned int)(std::rand() % aMax);

Nit: static_cast instead of C-style casts, everywhere.

@@ +103,5 @@
> +        std::set<int>::iterator it(mFds.begin());
> +        std::advance(it, Random(mFds.size()));
> +        FAULTY_LOG("trying to close collected pipe: %d\n", *it);
> +        int ret = close(*it);
> +        FAULTY_LOG("pipe status after attempt to close: %d\n", ret);

Shouldn't you remove this fd from the set now? Otherwise you could try to re-close it later.

@@ +161,5 @@
> +}
> +
> +void Faulty::MutateDouble(double *aValue)
> +{
> +    double limits[] = {DBL_MIN, DBL_MAX};

Nit: Might as well make this and all other min/max arrays static const

@@ +171,5 @@
> +            case 1:
> +                *aValue = RandomFloatingPointRange<double>(DBL_MIN, DBL_MAX);
> +                break;
> +        }
> +        return;

I don't understand how these Mutate* methods work... If mUseLargeValues is set then 62 out of 64 times this method won't actually mutate anything, right? Same for the MutateFloat method.

@@ +201,5 @@
> +                break;
> +        }
> +        return;
> +    }
> +    *aValue = std::rand() / 100000.0;

Hrm, shouldn't this be something like RAND_MAX?

@@ +229,5 @@
> +                break;
> +        }
> +        return;
> +    }
> +    FuzzIntegralType<int16_t>(aValue);

The compiler should be able to deduce the right type, no need to be explicit here.

@@ +455,5 @@
> +    }
> +    FuzzIntegralType<int64_t>(aValue);
> +}
> +
> +void Faulty::FuzzInt64(int64_t *aValue, unsigned int aProbability)

So, basically all of these Fuzz/Mutate methods are identical, they just have different types, limits, and a slightly different log message. It would be much cleaner to do this with templates...

::: ipc/glue/Faulty.h
@@ +18,5 @@
> +#include <set>
> +#include <fstream>
> +#include <cstddef>
> +#include <cmath>
> +#include <algorithm>

Several of these stl headers do not have any entries in config/stl-headers, so they may throw exceptions in some cases I guess. We need to review them for exception safety and add them to config/stl-headers.

@@ +21,5 @@
> +#include <cmath>
> +#include <algorithm>
> +#include "base/string16.h"
> +#include "base/string_util.h"
> +#include "base/singleton.h"

Nit: You've got a ton of includes here that can be moved to the cpp file. Please try to forward-declare everything that you can.

@@ +26,5 @@
> +
> +#define FAULTY_LOG(fmt, args...) printf_stderr("[Faulty] " fmt, ## args)
> +
> +// Decreasing the value, increases the chance of fuzzing a message.
> +const unsigned int DEFAULT_PROBABILITY = 1000;

Nit: Anything outside a namespace should be something like FAULTY_DEFAULT_PROBABLILTY or something. Though it looks like this should be moved to the cpp anyway.

@@ +29,5 @@
> +// Decreasing the value, increases the chance of fuzzing a message.
> +const unsigned int DEFAULT_PROBABILITY = 1000;
> +
> +namespace IPC {
> +    class Message;

This looks unnecessary

@@ +33,5 @@
> +    class Message;
> +}
> +
> +namespace mozilla {
> +namespace ipc {

It looks like all of these helper functions can be moved to your cpp file.

@@ +36,5 @@
> +namespace mozilla {
> +namespace ipc {
> +
> +template <class T>
> +T LargeValue(T base, T max)

Please add a static_assert for mozilla::IsArithmetic, see mfbt/TypeTraits.h.

@@ +38,5 @@
> +
> +template <class T>
> +T LargeValue(T base, T max)
> +{
> +    T x = (T)pow(base, (double)(rand() % max));

Nit: Here and below please use static_cast instead of c-style casts.

@@ +43,5 @@
> +    return x;
> +}
> +
> +template <class T>
> +T RandomFloatingPointRange(T min, T max)

Please add a static_assert for mozilla::IsFloatingPoint

@@ +51,5 @@
> +    return min + x * (max - min);
> +}
> +
> +template <class T>
> +T RandomIntegerRange(T min, T max)

Please add a static_assert for mozilla::IsIntegral

@@ +59,5 @@
> +    return x;
> +}
> +
> +template <typename T>
> +void FuzzIntegralType(T* v)

Please add a static_assert for mozilla::IsIntegral

@@ +85,5 @@
> +class Faulty
> +{
> +    public:
> +        void Init();
> +        bool IsValidProcessType();

IsValidProcessType() can be private.

@@ +87,5 @@
> +    public:
> +        void Init();
> +        bool IsValidProcessType();
> +        unsigned int GetSeed();
> +        void SetSeed(unsigned int aSeed);

I don't see any callers of GetSeed/SetSeed besides the constructor, can they be removed?

@@ +89,5 @@
> +        bool IsValidProcessType();
> +        unsigned int GetSeed();
> +        void SetSeed(unsigned int aSeed);
> +        unsigned int Random(unsigned int aMax);
> +        bool GetChance(unsigned int aProbability);

Random() and GetChance() can be private I think.

@@ +95,5 @@
> +        bool mFuzzPipes;
> +        bool mFuzzPickle;
> +        bool mUseLargeValues;
> +        bool mTargetChilds;
> +        bool mTargetParent;

Shouldn't these be private? Also, they should be const I think?

Nit: I find it much clearer when all the members are kept together rather than sprinkling methods before and after them.

@@ +97,5 @@
> +        bool mUseLargeValues;
> +        bool mTargetChilds;
> +        bool mTargetParent;
> +
> +        static unsigned int sDefaultProbability;

Nit: Any reason this can't be const with the number set here?

@@ +99,5 @@
> +        bool mTargetParent;
> +
> +        static unsigned int sDefaultProbability;
> +
> +        void MutateBool(bool *aValue);

All of the Mutate* methods can be private I believe.

@@ +149,5 @@
> +        void FuzzData(std::string& aData, int aLength, unsigned int aProbability=sDefaultProbability);
> +        void FuzzBytes(void *aData, int aLength, unsigned int aProbability=sDefaultProbability);
> +
> +        void CollectPipe(int aPipe);
> +        void CloseCollectedPipe(unsigned int aProbability=sDefaultProbability);

Can we combine all this into one function:

  MaybeCollectAndClosePipe(int aPipe, unsigned int aProbability=sDefaultProbability)

That would simplify the caller (ProcessOutgoingMessages) a lot.

@@ +154,5 @@
> +    private:
> +        Faulty();
> +        friend struct DefaultSingletonTraits<Faulty>;
> +        DISALLOW_EVIL_CONSTRUCTORS(Faulty);
> +        unsigned int mSeed;

It doesn't look like you actually need to save this value past the time when you call srandom and print to the log.

Comment 32

[Christoph Diehl [:posidron]]

Attachment: faulty.sh_v2.diff

Comment 33

[Christoph Diehl [:posidron]]

Attachment: faulty_v4.diff

Comment 34

[Christoph Diehl [:posidron]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #31)
> Comment on attachment 8343667 [details] [diff] [review]
> faulty_v3.diff

> ::: ipc/chromium/src/base/pickle.h
> @@ +12,5 @@
> >  #include "base/string16.h"
> >  
> > +#ifdef MOZ_FAULTY
> > +    #include "mozilla/ipc/Faulty.h"
> > +    #include "nsXULAppAPI.h"
> 
> Hm, why do you need nsXULAppAPI.h here?

The header nsXULAppAPI.h is needed by XRE_GetProcessType. I have moved the include now to Faulty.cpp

> @@ +171,5 @@
> > +            case 1:
> > +                *aValue = RandomFloatingPointRange<double>(DBL_MIN, DBL_MAX);
> > +                break;
> > +        }
> > +        return;
> 
> I don't understand how these Mutate* methods work... If mUseLargeValues is
> set then 62 out of 64 times this method won't actually mutate anything,
> right? Same for the MutateFloat method.

We have at the moment too many malloc_abort /NS_ABORT_OOM crashes which block more intense fuzzing.

> @@ +455,5 @@
> > +    }
> > +    FuzzIntegralType<int64_t>(aValue);
> > +}
> > +
> > +void Faulty::FuzzInt64(int64_t *aValue, unsigned int aProbability)
> 
> So, basically all of these Fuzz/Mutate methods are identical, they just have
> different types, limits, and a slightly different log message. It would be
> much cleaner to do this with templates...

That might be true but I would like to keep the flexibility for future uses.

Comment 35

[Dave Hylands [:dhylands]]

Comment on attachment 8346090 [details] [diff] [review]
faulty.sh_v2.diff

Review of attachment 8346090 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of minor comments.

::: faulty.sh
@@ +48,5 @@
> +adb wait-for-device
> +adb logcat -c
> +
> +if [[ $FAULTY_ENABLE_LOGGING = 1 ]] ; then
> +  export GDBSERVER_ENV="$GDBSERVER_ENV FAULTY_ENABLE_LOGGING=$FAULTY_ENABLE_LOGGING "

nit: You only need to export the variable once.

Since you export at the end of the script, you can drop all of the exports from inside the ifs. FYI - you can also do export FOO with no = and it will export the variable.

Should you be initializing GDBSERVER_ENV? Without it you'll append to whatever is in the callers environment (which may be what you want - I wasn't sure)

@@ +68,5 @@
> +  export GDBSERVER_ENV="$GDBSERVER_ENV FAULTY_PARENT=$FAULTY_PARENT "
> +fi
> +
> +if [[ $FAULTY_LARGE_VALUES = 1 ]] ; then
> +	export GDBSERVER_ENV="$GDBSERVER_ENV FAULTY_LARGE_VALUES=$FAULTY_LARGE_VALUES "

nit: indentation (maybe a tab instead of spaces?)

Comment 36

[Dave Hylands [:dhylands]]

Comment on attachment 8346190 [details] [diff] [review]
faulty_v4.diff

Review of attachment 8346190 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple minor comments.

::: ipc/glue/Faulty.cpp
@@ +72,5 @@
> +void FuzzStringType(T& v, const T& literal1, const T& literal2)
> +{
> +  switch (rand() % 5) {
> +    case 4:
> +      v = v + v;

nit: Add a comment indicating that the missing break was intentional.

I normally just do something like:

// fall through

@@ +118,5 @@
> +  FAULTY_LOG("Strategy: pipe   = %s", mFuzzPipes ? "enabled" : "disabled");
> +}
> +
> +bool
> +Faulty::IsValidProcessType(void)

nit: I like to see a comment

// static

that serves to indicate that this is a static function. I also like to add // virtual for virtual functions (you don't have any here - jsut though I'd mention it)

Comment 37

[Christoph Diehl [:posidron]]

Attachment: faulty_v5.diff

Fixes nits of comment #36

Comment 38

[Christoph Diehl [:posidron]]

Attachment: faulty.sh_v3.diff

Fixes nits of comment #35

Comment 39

[Dave Hylands [:dhylands]]

Comment on attachment 8357215 [details] [diff] [review]
faulty_v5.diff

Review of attachment 8357215 [details] [diff] [review]:
-----------------------------------------------------------------

Changing feedback? back to + since cdiehl told me he changed it by accident.

Comment 40

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8357215 [details] [diff] [review]
faulty_v5.diff

Review of attachment 8357215 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: ipc/glue/Faulty.cpp
@@ +10,5 @@
> +#include "chrome/common/ipc_message.h"
> +#include "chrome/common/ipc_channel.h"
> +#include "prenv.h"
> +#include "mozilla/TypeTraits.h"
> +#include "mozilla/ipc/Faulty.h"

Please make sure your header is always the first included in the cpp, otherwise you might not notice if you make the header dependent on something else.

@@ +104,5 @@
> +  FAULTY_LOG("Initializing.");
> +
> +  // This is not a guranteed way to reproduce a crash but can help.
> +  const char* userSeed = PR_GetEnv("FAULTY_SEED");
> +  unsigned long randomSeed = static_cast<unsigned long>(std::time(0));

PR_IntervalNow would make me more comfortable, we don't really use std::time anywhere else.

@@ +106,5 @@
> +  // This is not a guranteed way to reproduce a crash but can help.
> +  const char* userSeed = PR_GetEnv("FAULTY_SEED");
> +  unsigned long randomSeed = static_cast<unsigned long>(std::time(0));
> +  if (userSeed) {
> +    long n = std::strtol(userSeed, (char**)nullptr, 10);

Please use c++ static_cast

::: ipc/glue/Faulty.h
@@ +16,5 @@
> +#define FAULTY_DEFAULT_PROBABILITY 1000
> +#define FAULTY_LOG(fmt, args...) \
> + if (mozilla::ipc::Faulty::IsLoggingEnabled()) { \
> +  printf_stderr("[Faulty] " fmt, ## args); \
> + }

Nit: 2 space indent

@@ +32,5 @@
> +  public:
> +    // Used as a default argument for the Fuzz|datatype| methods.
> +    static const unsigned int sDefaultProbability;
> +
> +    static unsigned int DefaultProbability(void);

Doesn't look like this needs to be a static method, how about just another function in your cpp? Same for IsValidProcessType().

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8357224 [details] [diff] [review]
faulty.sh_v3.diff

Review of attachment 8357224 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, I'm not sure who should review this, but my knowledge of bash is so poor that I don't feel comfortable doing it.

Maybe dhylands can take this?

Comment 42

[Dave Hylands [:dhylands]]

Comment on attachment 8357224 [details] [diff] [review]
faulty.sh_v3.diff

Review of attachment 8357224 [details] [diff] [review]:
-----------------------------------------------------------------

::: faulty.sh
@@ +48,5 @@
> +adb wait-for-device
> +adb logcat -c
> +
> +if [[ $FAULTY_ENABLE_LOGGING = 1 ]] ; then
> +  GDBSERVER_ENV="$GDBSERVER_ENV FAULTY_ENABLE_LOGGING=$FAULTY_ENABLE_LOGGING "

nit: You don't need the trailing space here, or on the rest of the assignments to GDBSERVER_ENV

You're adding the space already (after GDBSERVER_ENV and before SOMETHING) "$GDBSERVER_ENV SOMETHING"

Comment 43

[Christoph Diehl [:posidron]]

Attachment: faulty.sh_v4.diff

Fix nits from comment 42.

Comment 44

[Christoph Diehl [:posidron]]

Attachment: faulty_v6.diff

Fix nits from comment 40.

Bent, I really would like DefaultProbability() and IsValidProcessType() to be a part of the class Faulty.

Since you +'ed the patch and those were nits, I will carry on the + for this patch. I hope that's okay.

Comment 45

[Christoph Diehl [:posidron]]

Comment on attachment 8364252 [details] [diff] [review]
faulty.sh_v4.diff

Same here; carrying on the + from dhylands.

Comment 46

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8364338 [details] [diff] [review]
faulty_v6.diff

If you feel strongly then I'm ok with it.

Comment 47

[Christoph Diehl [:posidron]]

Attachment: faulty_v7.diff

Slight adjustments to make the fuzzer runnable with desktop builds.

Comment 48

[Christoph Diehl [:posidron]]

Attachment: faulty_v8.diff

Remove two forgotten and unnecessary trailing newline characters.

Comment 49

[Benoit Jacob [:bjacob] (mostly away)]

Let me know if I should stop blocking this bug by all the bugs that I find with Faulty. This fuzzer is a real gold mine. I realize I'm causing a lot of spam, so let me know.

Comment 50

[Christoph Diehl [:posidron]]

Keep going!

Comment 51

[Christoph Diehl [:posidron]]

Attachment: faulty_v9.diff

- Fix: update to trunk
- Fix: prevent integer overflows while generating integrals
- Fix: use random() instead of rand() when initializing with srandom()
- Fix: prevent leakage of large values even if LARGE_VALUES is not set
- Fix: reformat lines to max 80 chars / line
- Add: improve generation of double and float values
- Add: make IPC's DCHECK assertion macro non-fatal

Comment 52

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Congratulations, Christoph!  Great to see faulty put to such good use :).

Comment 53

[Christoph Diehl [:posidron]]

Attachment: faulty_v10.diff

Removing a - now obsolete - send_msg() debug print.

Comment 54

[Haik Aftandilian [:haik]]

Attachment: (fuzzing-ipc-ipdl) Fuzzing: IPC Protocol Definition Language (IPDL) Protocols

Made a few tweaks to get this to compile on mozilla-central.

Comment 55

[Andrew McCreight [:mccr8]]

(bzexport assigns bugs to you by default when you upload something using it for some reason.)

Comment 56

[Christoph Diehl [:posidron]]

Awesome! Thanks Haik for continuing this work!

I doubt it's necessary but let me put here comment #1 of bug 1078105 which should help you in getting things running and fuzzing.


Fuzz GMP with Faulty

https://bugzilla.mozilla.org/show_bug.cgi?id=777067   - faulty_v*.diff
https://gist.github.com/posidron/63871275abfcf92ebc18 - mozconfig.mi-asan-opt-faulty
https://gist.github.com/posidron/6ba4327c5605961496bd - mozconfig.fuzzing.common
https://gist.github.com/posidron/a74118edcfed1910e141 - user.js
https://gist.github.com/posidron/e04cd3d28c6163abee0f - faulty.sh
https://gist.github.com/posidron/5ef64f1547ca61ae5782 - gmp-testcase.html

1] Apply the patch faulty.diff to trunk
2] Compile Firefox "export MOZCONFIG=mozconfig.mi-asan-opt-faulty"
3] Create a Firefox profile named "fuzzing"
4] Move the user.js preferences to the "fuzzing" profile
5] Launch Faulty "./faulty.sh gmp-testcase.html"

Comment 57

[Christoph Diehl [:posidron]]

Attachment: faulty.v13.diff

Update for making it work with:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5191bafd5e34

Comment 58

[Christoph Diehl [:posidron]]

I would like to run this fuzzer (autonomous) with rr at DigitalOcean, in order to reproduce bugs more easier for developers but this is going to be impossible to realize if at every second pull something changed and I need to manually update the patch set.

Therefore I would like to ask again to add this upstream into Firefox or provide a reasonable explanation why this should not be done.

Comment 59

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like it got reviewed. I don't see any reason why it can't be landed. Do you have commit rights Christoph?

Comment 60

[Christoph Diehl [:posidron]]

I believe I only have commit level 2 access.

Comment 61

[Christoph Diehl [:posidron]]

(In reply to Christoph Diehl [:posidron] from comment #60)
> I believe I only have commit level 2 access.

I definitely know that I do not have commit level 3 access. :-)

Comment 62

[Christoph Diehl [:posidron]]

Attachment: faulty.v14.diff

Update for making it work with:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8a9f6e77e8

Comment 63

[Carsten Book [:Tomcat]]

Hi Christoph,

just wondering what patches do you want to get checked in, just  faulty.sh_v4.diff (2.28 KB, patch)  ? 

since (fuzzing-ipc-ipdl) Fuzzing: IPC Protocol Definition Language (IPDL) Protocols and faulty.v14.diff still waiting for review or ?

Comment 64

[Christoph Diehl [:posidron]]

I think the patch "faulty.v14.diff" would be enough for the start. The other scripts can be downloaded via GitHub, cause those are mostly individual bash scripts for automation.

Comment 65

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8758070 [details] [diff] [review]
faulty.v14.diff

Review of attachment 8758070 [details] [diff] [review]:
-----------------------------------------------------------------

I think this patch could use some cleanup, but I don't want to hold it up. Let's just land it.

Comment 66

[Carsten Book [:Tomcat]]

(In reply to Bill McCloskey (:billm) from comment #65)
> Comment on attachment 8758070 [details] [diff] [review]
> faulty.v14.diff
> 
> Review of attachment 8758070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this patch could use some cleanup, but I don't want to hold it up.
> Let's just land it.

setting keyword for the other patches

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0e3af9742f
"Fuzzing: IPC Protocol Definition Language (IPDL) Protocols". r=wmccloskey

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e0e3af9742f

Comment 69

[Christoph Diehl [:posidron]]

We need to discuss how we can improve fuzzing IPC/Sandbox and how to move forward, most of the people are CC'ed here. We need a clear direction and plan in how to proceed. Do we want to improve Faulty or do we want to create a new project for on-demand fuzzing. The cons/pros are normally clear. I need a person who literally will sit down, commit time and work on future steps.

Comment 70

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Christoph Diehl [:posidron] from comment #69)
> We need to discuss how we can improve fuzzing IPC/Sandbox and how to move
> forward, most of the people are CC'ed here. We need a clear direction and
> plan in how to proceed. Do we want to improve Faulty or do we want to create
> a new project for on-demand fuzzing. The cons/pros are normally clear. I
> need a person who literally will sit down, commit time and work on future
> steps.

My team is keen to help (cr, julian, steph & kate). They are now cc'd on this bug, and I'll arrange a catch up to figure a plan.

Comment 71

[Mike Hommey [:glandium]]

FWIW, build system peers should have been looped in on the configure changes from this bug before landing. Specifically, those changes would have been r-ed as adding a new configure flag to old-configure.

Comment 72

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Hommey [:glandium] from comment #71)
> FWIW, build system peers should have been looped in on the configure changes
> from this bug before landing. Specifically, those changes would have been
> r-ed as adding a new configure flag to old-configure.

Sorry about this Mike. But I have to say, it would really help a lot if there was more information on what is supposed to replace configure. What I've seen on dev-platform has mostly just said that configure is going away. What are we supposed to use instead? Is there documentation?

Comment 73

[Mike Hommey [:glandium]]

The message to dev-platform also said to come talk to build peers if you want to touch configure.

Comment 74

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Hommey [:glandium] from comment #73)
> The message to dev-platform also said to come talk to build peers if you
> want to touch configure.

OK, mea culpa. But what should we do instead?

Comment 75

[Mike Hommey [:glandium]]

Ask build peers when you need to touch configure :) Don't worry about /this/ occurrence, I already have a patch taking care of it.

Comment 76

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Hommey [:glandium] from comment #75)
> Ask build peers when you need to touch configure :) Don't worry about /this/
> occurrence, I already have a patch taking care of it.

Can I see the patch? That way, the next time this happens, I can do the right thing and ask you for review. That's quicker than a needinfo for how to do it followed by review, which is how I would have to do it otherwise. I'm sure you're very fast responding to these things, but one round of feedback is always better than two.

Comment 80

[Christoph Diehl [:posidron]]

Attachment: faulty.v15.diff

What's new in Faulty v15

* Move Faulty from 'ipc/glue' to 'tools/fuzzing/faulty'
* Improve determination of the attack process
** Valid attack processes are: Plugin, Content, GMPPlugin, GPU, PDFium
* Add blacklist support to prevent messages from being fuzzed or to common crashes
* Add whitelist support to fuzz specific messages only
* Add functionality to dump messages
* Add functionality to mutate message blobs and save both original and mutated message to disk
* Add mutation factor which determines by the size of the message, the amount of mutations for a message
* Remove some pickle fuzzing functions of which the counterparts were not existing anymore
* Improve output visuals for better log viewing

Newly added Env variables:
    * FAULTY_BLACKLIST= path to a blacklist file
    * FAULTY_AS_WHITELIST= use the blacklist file as whitelist
    * FAULTY_MUTATION_FACTOR= number, as described above
    * FAULTY_MESSAGE_PATH= dump path for message test-cases
    * FAULTY_MESSAGES= additional strategy option to pickle|pipe


Known issues:

ReadFile() inside IsMessageBlacklisted() ignores the first 3 messages in each new process. It looks like a timing related issue, I could figure out. The first three messages are usually:
  * <unknown IPC msg name>
  * PContent::Msg_BackUpXResources
  * SHMEM_CREATED_MESSAGE

rv = fileStream->Init(file, -1, -1, false);

Is the function which fails on the first three tries.


Test commands with blacklist:

FAULTY_MUTATION_FACTOR=30 FAULTY_MESSAGES=1 FAULTY_BLACKLIST="`pwd`/blacklist.txt" FAULTY_MESSAGE_PATH='/tmp/faulty' FAULTY_PROBABILITY=10 FAULTY_PARENT=1 FAULTY_ENABLE_LOGGING=1 xvfb-run ./mach crashtest


Test commands with whitelist:

FAULTY_MUTATION_FACTOR=30 FAULTY_MESSAGES=1 FAULTY_BLACKLIST="`pwd`/whitelist.txt" FAULTY_AS_WHITELIST=1 FAULTY_MESSAGE_PATH='/tmp/faulty' FAULTY_PROBABILITY=10 FAULTY_PARENT=1 FAULTY_ENABLE_LOGGING=1 xvfb-run ./mach crashtest

Comment 81

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8963898 [details] [diff] [review]
faulty.v15.diff

Review of attachment 8963898 [details] [diff] [review]:
-----------------------------------------------------------------

A few minor things:

::: ipc/glue/Faulty.cpp
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

The moved files should use `hg mv` to record that they've moved, so they'll show up in diffs as a move with edits instead of deleting one file and adding an entirely new one.  `hg mv --after` should work to retroactively mark moves like this.

(I imported the patch into Git, which does heuristic rename detection instead, so *I* can see the diff, but fixing that before this lands will help anyone trying to follow the history in Hg.)

::: tools/fuzzing/faulty/Faulty.cpp
@@ +277,5 @@
> +  } else if (currentProcessType == GeckoProcessType_Plugin
> +          || currentProcessType == GeckoProcessType_Content
> +          || currentProcessType == GeckoProcessType_GMPlugin
> +          || currentProcessType == GeckoProcessType_GPU
> +          || currentProcessType == GeckoProcessType_PDFium) {

I'm a little confused by what these conditionals are doing — it looks like we'll still set isValidProcessType in these process types if (!targetChildren && !targetParent)?

@@ +691,5 @@
> +      return rv;
> +    }
> +  }
> +
> +  path.forget();

Why are you leaking the path?

@@ +722,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  rv = fileStream->Init(file, -1	, -1, false);

Nit: that last argument is actually an integer containing bit flags; it used to be a boolean a long time ago, and there are still call sites that pass false because nobody's gotten around to changing them, but strictly speaking this should be 0.

As for why this is failing when called early: that seems weird, so I took a look at the implementation and the only thing I can think of is if maybe the open() is failing with EINTR.  According to the Linux man page that should only happen for “slow” files like FIFOs (or tape drives, or maybe files on NFS mounts with the `intr` flag?), but maybe that's outdated.  Unfortunately the error code is buried under a couple layers of abstraction, and NS_ErrorAccordingToNSPR doesn't handle PR_PENDING_INTERRUPT_ERROR specially (and there doesn't seem to be a useful nsresult to map it to).  You might want to try using something like stdio that allows checking errno directly.

@@ +759,5 @@
> +
> +  if (!sFileLoaded && mBlacklistPath) {
> +    if (ReadFile(mBlacklistPath, sMessageBlacklist) != NS_OK) {
> +        FAULTY_LOG("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
> +  FAULTY_LOG("Contains: %d", sMessageBlacklist.Contains(aMessageName));

This is in the error case, so won't sMessageBlacklist always be empty here?

@@ +866,5 @@
> +  }
> +
> +  /* Build new message. */
> +  auto *mutatedMsg = new IPC::Message(reinterpret_cast<const char*>(data.data()), data.size());
> +  CopyFDs(mutatedMsg, aMsg);

Shouldn't there be a `delete aMsg` somewhere in here?  It looks like this will leak the original message.

Comment 82

[Christoph Diehl [:posidron]]

Attachment: faulty.v15.1.diff

Fixed issues from above.

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e4fec63d98
Fuzzing: IPC Protocol Definition Language (IPDL) Protocols. r=jld

Comment 84

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/44e4fec63d98

Comment 85

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:posidron, maybe it's time to close this bug?

Comment 86

[Christoph Diehl [:posidron]]

Huh, definitely not. :-)

The project stalled cause I was busy with other things but we are working on new harness improvements during the All Hands, on Windows ASan builds and Faulty will very likely also be offered for my new crowdfuzz project.

There will be more bugs.

Comment 87

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:posidron, maybe it's time to close this bug?

Comment 88

[Christoph Diehl [:posidron]]

I would prefer to keep it open, closing it gives the wrong signals. We focused lately on the LibFuzzer implementation of IPCContent - moving it to OSSFuzz, eliminating and blacklisting further startup crashes; we will come back to this one.

Comment 89

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:decoder, could you have a look please?
For more information, please visit auto_nag documentation.