Open Bug 777067 (fuzzing-ipc-ipdl) Opened 7 years ago Updated 6 months ago

[meta] Fuzzing: IPC Protocol Definition Language (IPDL) Protocols

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

ASSIGNED
blocking-kilimanjaro +
blocking-basecamp -

People

(Reporter: sicking, Assigned: posidron)

References

(Depends on 27 open bugs, Blocks 4 open bugs, )

Details

(Keywords: leave-open, meta, sec-other, Whiteboard: [soft] )

Attachments

(2 files, 20 obsolete files)

2.28 KB, patch
posidron
: review+
Details | Diff | Splinter Review
31.66 KB, patch
jld
: review+
Details | Diff | Splinter Review
No description provided.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 516716
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
It doesn't really matter to me.
what Dan said was what was intended.
blocking-basecamp: --- → +
Whiteboard: [soft]
Depends on: 778382
Depends on: 779988
Depends on: 780219
Renom if you think we can't ship a v1 without this.
blocking-basecamp: + → ---
blocking-kilimanjaro: --- → +
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
blocking-basecamp: --- → ?
Depends on: 781594
Depends on: 781903
Assignee: nobody → cdiehl
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.
blocking-basecamp: ? → -
Depends on: 782940
Depends on: 782945
Depends on: 807262
Depends on: 807263
Depends on: 807264
Depends on: 807266
Depends on: 807738
Depends on: 808080
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.
Group: core-security
Keywords: sec-other
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.
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.
(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?
(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
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.  :-)
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.
Alias: fuzzing-ipc-ipdl
OS: Mac OS X → All
Hardware: x86 → All
Summary: Fuzz all ipdl protocols → Fuzzing: IPC Protocol Definition Language (IPDL) Protocols
Status: REOPENED → NEW
Depends on: 931086
Depends on: 933820
Depends on: 933821
Depends on: 933822
Depends on: 933823
Depends on: 933824
Depends on: 933825
Depends on: 933826
Depends on: 933827
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!
Attached patch faulty.diff (obsolete) — Splinter Review
Daniel Veditz and I talked about a possible integration of the fuzzer in our tree - hidden behind a build flag.
Attached patch default-gecko-config.diff (obsolete) — Splinter Review
Attached patch faulty-tool.diff (obsolete) — Splinter Review
Ben, could you find a person to review those patches?
Flags: needinfo?(bent.mozilla)
Group: core-security
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 on attachment 8341916 [details] [diff] [review]
default-gecko-config.diff

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

Looks fine to me.
Attachment #8341916 - Flags: review+
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)
Attached patch faulty.diff (obsolete) — Splinter Review
Thank you very much for your comments dhylands.
Attachment #8341915 - Attachment is obsolete: true
Attachment #8342295 - Flags: review?(dhylands)
Attached patch faulty.sh.diff (obsolete) — Splinter Review
Attachment #8341918 - Attachment is obsolete: true
Attachment #8342308 - Flags: review?(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).
Attachment #8342308 - Flags: review?(dhylands) → review+
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
Attachment #8342295 - Flags: review?(dhylands) → feedback+
Attached patch faulty_v3.diff (obsolete) — Splinter Review
Fixed nits from comment 27.
Attachment #8343665 - Flags: review?(dhylands)
Attached patch faulty_v3.diff (obsolete) — Splinter Review
Attachment #8343665 - Attachment is obsolete: true
Attachment #8343665 - Flags: review?(dhylands)
Attachment #8343667 - Flags: review?(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.
Attachment #8343667 - Flags: review?(dhylands)
Attachment #8343667 - Flags: review?(bent.mozilla)
Attachment #8343667 - Flags: feedback+
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.
Attachment #8343667 - Flags: review?(bent.mozilla)
Flags: needinfo?(bent.mozilla)
Attached patch faulty.sh_v2.diff (obsolete) — Splinter Review
Attachment #8342308 - Attachment is obsolete: true
Attachment #8346090 - Flags: review?(bent.mozilla)
Attachment #8346090 - Flags: feedback?(dhylands)
Attached patch faulty_v4.diff (obsolete) — Splinter Review
Attachment #8342295 - Attachment is obsolete: true
Attachment #8343667 - Attachment is obsolete: true
Attachment #8346190 - Flags: review?(bent.mozilla)
Attachment #8346190 - Flags: feedback?(dhylands)
(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 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?)
Attachment #8346090 - Flags: feedback?(dhylands) → feedback+
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)
Attachment #8346190 - Flags: feedback?(dhylands) → feedback+
Attached patch faulty_v5.diff (obsolete) — Splinter Review
Fixes nits of comment #36
Attachment #8346190 - Attachment is obsolete: true
Attachment #8346190 - Flags: review?(bent.mozilla)
Attachment #8357215 - Flags: review?(bent.mozilla)
Attachment #8357215 - Flags: feedback?(dhylands)
Attached patch faulty.sh_v3.diff (obsolete) — Splinter Review
Fixes nits of comment #35
Attachment #8346090 - Attachment is obsolete: true
Attachment #8346090 - Flags: review?(bent.mozilla)
Attachment #8357224 - Flags: review?(bent.mozilla)
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.
Attachment #8357215 - Flags: feedback?(dhylands) → feedback+
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().
Attachment #8357215 - Flags: review?(bent.mozilla) → review+
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?
Attachment #8357224 - Flags: review?(bent.mozilla) → review?(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"
Attachment #8357224 - Flags: review?(dhylands) → review+
Fix nits from comment 42.
Attachment #8357224 - Attachment is obsolete: true
Attached patch faulty_v6.diff (obsolete) — Splinter Review
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.
Attachment #8357215 - Attachment is obsolete: true
Attachment #8364338 - Flags: review+
Comment on attachment 8364252 [details] [diff] [review]
faulty.sh_v4.diff

Same here; carrying on the + from dhylands.
Attachment #8364252 - Flags: review+
Comment on attachment 8364338 [details] [diff] [review]
faulty_v6.diff

If you feel strongly then I'm ok with it.
Attached patch faulty_v7.diff (obsolete) — Splinter Review
Slight adjustments to make the fuzzer runnable with desktop builds.
Attachment #8364338 - Attachment is obsolete: true
Attached patch faulty_v8.diff (obsolete) — Splinter Review
Remove two forgotten and unnecessary trailing newline characters.
Attachment #8365596 - Attachment is obsolete: true
No longer depends on: 967528
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.
Depends on: 968004
Keep going!
Attached patch faulty_v9.diff (obsolete) — Splinter Review
- 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
Attachment #8365602 - Attachment is obsolete: true
Attachment #8495079 - Flags: review+
Depends on: 1072867
Depends on: 1072871
Depends on: 1072877
Congratulations, Christoph!  Great to see faulty put to such good use :).
Depends on: 1073345
Depends on: 1073350
Attached patch faulty_v10.diff (obsolete) — Splinter Review
Removing a - now obsolete - send_msg() debug print.
Attachment #8495079 - Attachment is obsolete: true
Depends on: 1081956
Depends on: 1081960
Depends on: 1081961
Depends on: 1081965
Depends on: 1232330
No longer depends on: 1232330
Made a few tweaks to get this to compile on mozilla-central.
Assignee: cdiehl → haftandilian
Status: NEW → ASSIGNED
(bzexport assigns bugs to you by default when you upload something using it for some reason.)
Assignee: haftandilian → cdiehl
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"
Attached patch faulty.v13.diff (obsolete) — Splinter Review
Update for making it work with:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5191bafd5e34
Attachment #8504045 - Attachment is obsolete: true
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.
It looks like it got reviewed. I don't see any reason why it can't be landed. Do you have commit rights Christoph?
I believe I only have commit level 2 access.
(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. :-)
Attached patch faulty.v14.diff (obsolete) — Splinter Review
Update for making it work with:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8a9f6e77e8
Attachment #8756125 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8758070 - Flags: review?(bent.mozilla)
Attachment #8758070 - Flags: checkin?
Attachment #8758070 - Flags: checkin?
Keywords: checkin-needed
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 ?
Flags: needinfo?(cdiehl)
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.
Flags: needinfo?(cdiehl)
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.
Attachment #8758070 - Flags: review?(bent.mozilla) → review+
Flags: needinfo?(wmccloskey)
(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
Keywords: leave-open
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0e3af9742f
"Fuzzing: IPC Protocol Definition Language (IPDL) Protocols". r=wmccloskey
Keywords: checkin-needed
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.
(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.
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.
(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?
The message to dev-platform also said to come talk to build peers if you want to touch configure.
(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?
Ask build peers when you need to touch configure :) Don't worry about /this/ occurrence, I already have a patch taking care of it.
(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.
Blocks: 1294630
Depends on: 1301357
Depends on: 1337697
Depends on: 1339306
Duplicate of this bug: 516716
See Also: → 1359755
Depends on: 1382234
Depends on: 1384858
Depends on: 1386787
Depends on: 1388020
Depends on: 1392739
Depends on: 1423245
Depends on: 1423251
Depends on: 1433609
Depends on: 1445234
Depends on: 1445238
Depends on: 1445438
Depends on: 1445440
Depends on: 1445443
Depends on: 1445447
Depends on: 1445467
Depends on: 1445472
Depends on: 1445485
Depends on: 1446021
Depends on: 1446022
Depends on: 1446108
Duplicate of this bug: 782982
Duplicate of this bug: 777069
Depends on: 1447425
Depends on: 1449405
Depends on: 1449420
Depends on: 1449530
Depends on: 1449736
Attached patch faulty.v15.diff (obsolete) — Splinter Review
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
Attachment #8341916 - Attachment is obsolete: true
Attachment #8699238 - Attachment is obsolete: true
Attachment #8758070 - Attachment is obsolete: true
Attachment #8963898 - Flags: review?(jld)
Depends on: 1450231
Depends on: 1450232
Depends on: 1450942
Depends on: 1451294
Depends on: 1451295
Depends on: 1451297
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.
Attachment #8963898 - Flags: review?(jld) → review+
Fixed issues from above.
Attachment #8963898 - Attachment is obsolete: true
Attachment #8970644 - Flags: superreview?(jld)
Attachment #8970644 - Flags: superreview?(jld) → review+
Keywords: checkin-needed
Whiteboard: [soft] → [soft]
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e4fec63d98
Fuzzing: IPC Protocol Definition Language (IPDL) Protocols. r=jld
Keywords: checkin-needed
Depends on: 1467468
The leave-open keyword is there and there is no activity for 6 months.
:posidron, maybe it's time to close this bug?
Flags: needinfo?(cdiehl)
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.
Flags: needinfo?(cdiehl)
Depends on: 1509500
Summary: Fuzzing: IPC Protocol Definition Language (IPDL) Protocols → [meta] Fuzzing: IPC Protocol Definition Language (IPDL) Protocols
Depends on: 1517542
Depends on: 1519232
Depends on: 1520873
Depends on: 1535671
You need to log in before you can comment on or make changes to this bug.