Closed Bug 1382234 Opened 2 years ago Closed 2 years ago

Disable MOZ_PICKLE_SENTINEL_CHECKING in --enable-fuzzing builds

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: posidron, Assigned: posidron)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch disable-sentinel-checks.diff (obsolete) — Splinter Review
Most of the crashes which occur while fuzzing IPC are aborting because of sentinel mismatches. With this patch I am considering to disable those checks for our fuzzing builds but I would like to hear some opinions about whether this could cause some possible unknown side effects.

Besides that I am a bit confused regarding bug 1348708 ...


http://searchfox.org/mozilla-central/source/__GENERATED__/ipc/ipdl/PHttpChannelParent.cpp#686

// Sentinel = 'data'
if ((!(((&(msg__)))->ReadSentinel((&(iter__)), 843352540)))) {
    mozilla::ipc::SentinelReadError("Error deserializing 'nsCString'");
    return MsgValueError;
}



http://searchfox.org/mozilla-central/source/ipc/chromium/src/base/pickle.h#26

#if !defined(RELEASE_OR_BETA) || defined(DEBUG)
#define MOZ_PICKLE_SENTINEL_CHECKING
#endif

http://searchfox.org/mozilla-central/source/ipc/chromium/src/base/pickle.cc#453

#ifdef MOZ_PICKLE_SENTINEL_CHECKING
bool Pickle::ReadSentinel(PickleIterator* iter, uint32_t sentinel) const {
  uint32_t found;
  if (!ReadUInt32(iter, &found)) {
    return false;
  }
  return found == sentinel;
}



http://searchfox.org/mozilla-central/source/ipc/glue/ProtocolUtils.cpp#360

void
SentinelReadError(const char* aClassName)
{
  nsPrintfCString message("incorrect sentinel when reading %s", aClassName);
  NS_RUNTIMEABORT(message.get());
}


http://searchfox.org/mozilla-central/source/xpcom/base/nsDebug.h#247

** These need to be compiled regardless of the DEBUG flag.

[..]

#define NS_RUNTIMEABORT(msg)                                    \
  NS_DebugBreak(NS_DEBUG_ABORT, msg, nullptr, __FILE__, __LINE__)


If MOZ_PICKLE_SENTINEL_CHECKING only gets enabled in non-release and debug builds and only then Pickle::ReadSentinel is enabled / compiled how did we end up with a sentinel mismatch?
Attachment #8887947 - Flags: feedback?(wmccloskey)
Attachment #8887947 - Flags: feedback?(cyu)
Assignee: nobody → cdiehl
Comment on attachment 8887947 [details] [diff] [review]
disable-sentinel-checks.diff

LGTM.
Attachment #8887947 - Flags: feedback?(cyu) → feedback+
Comment on attachment 8887947 [details] [diff] [review]
disable-sentinel-checks.diff

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

::: ipc/chromium/src/base/pickle.h
@@ +20,5 @@
>  #ifdef FUZZING
>  #include "base/singleton.h"
>  #include "mozilla/ipc/Faulty.h"
>  #endif
> +#if !defined(RELEASE_OR_BETA) && !defined(FUZZING) || defined(DEBUG)

Please use parentheses here. People shouldn't have to know the precedences of && and ||.
Attachment #8887947 - Flags: feedback?(wmccloskey) → feedback+
(In reply to Christoph Diehl [:posidron] from comment #0)
> If MOZ_PICKLE_SENTINEL_CHECKING only gets enabled in non-release and debug
> builds and only then Pickle::ReadSentinel is enabled / compiled how did we
> end up with a sentinel mismatch?

MOZ_PICKLE_SENTINEL_CHECKING is enabled in DEBUG build *or* in non-release builds. So it's enabled in Nightlies. That's where the crash report is coming from.
Fixed issue in Comment 2
Attachment #8887947 - Attachment is obsolete: true
Attachment #8889031 - Flags: review?(wmccloskey)
Attachment #8889031 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98cebc8c28cc
Disable MOZ_PICKLE_SENTINEL_CHECKING in --enable-fuzzing builds. r=billm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/98cebc8c28cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.