Skip to content

Commit

Permalink
Bug 1754004 - Part 11: Simplify the IPCStream serialization API, r=as…
Browse files Browse the repository at this point in the history 
…uth,necko-reviewers,kershaw

As serializing IPCStream no longer requires a manager or FileDescriptor array,
the arguments are no longer necessary, and can be removed. The AutoIPCStream
helper can also be removed, as managed actors are no longer used for
serialization, so a delayed start callback is not necessary.

The delayed start parameter is also removed from nsIIPCSerializableInputStream
instances, but is still present as `aAllowLazy` on the toplevel serialization
methods.

Differential Revision: https://phabricator.services.mozilla.com/D141048
  • Loading branch information
@mystor
mystor committed May 13, 2022
1 parent 4a47497 commit 90aa76c
Show file tree
Hide file tree
Showing 62 changed files with 353 additions and 1,444 deletions.
54 changes: 8 additions & 46 deletions dom/cache/AutoUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
using mozilla::Maybe;
using mozilla::Unused;
using mozilla::dom::cache::CacheReadStream;
using mozilla::ipc::AutoIPCStream;
using mozilla::ipc::PBackgroundParent;

namespace {
Expand Down Expand Up @@ -55,12 +54,6 @@ AutoChildOpArgs::AutoChildOpArgs(TypeUtils* aTypeUtils,
: mTypeUtils(aTypeUtils), mOpArgs(aOpArgs), mSent(false) {
MOZ_DIAGNOSTIC_ASSERT(mTypeUtils);
MOZ_RELEASE_ASSERT(aEntryCount != 0);
// We are using AutoIPCStream objects to cleanup target IPCStream
// structures embedded in our CacheOpArgs. These IPCStream structs
// must not move once we attach our AutoIPCStream to them. Therefore,
// its important that any arrays containing streams are pre-sized for
// the number of entries we have in order to avoid realloc moving
// things around on us.
if (mOpArgs.type() == CacheOpArgs::TCachePutAllArgs) {
CachePutAllArgs& args = mOpArgs.get_CachePutAllArgs();
args.requestResponseList().SetCapacity(aEntryCount);
Expand Down Expand Up @@ -117,8 +110,6 @@ AutoChildOpArgs::~AutoChildOpArgs() {
// Other types do not need cleanup
break;
}

mStreamCleanupList.Clear();
}

void AutoChildOpArgs::Add(const InternalRequest& aRequest,
Expand All @@ -130,37 +121,35 @@ void AutoChildOpArgs::Add(const InternalRequest& aRequest,
case CacheOpArgs::TCacheMatchArgs: {
CacheMatchArgs& args = mOpArgs.get_CacheMatchArgs();
mTypeUtils->ToCacheRequest(args.request(), aRequest, aBodyAction,
aSchemeAction, mStreamCleanupList, aRv);
aSchemeAction, aRv);
break;
}
case CacheOpArgs::TCacheMatchAllArgs: {
CacheMatchAllArgs& args = mOpArgs.get_CacheMatchAllArgs();
MOZ_DIAGNOSTIC_ASSERT(args.maybeRequest().isNothing());
args.maybeRequest().emplace(CacheRequest());
mTypeUtils->ToCacheRequest(args.maybeRequest().ref(), aRequest,
aBodyAction, aSchemeAction, mStreamCleanupList,
aRv);
aBodyAction, aSchemeAction, aRv);
break;
}
case CacheOpArgs::TCacheDeleteArgs: {
CacheDeleteArgs& args = mOpArgs.get_CacheDeleteArgs();
mTypeUtils->ToCacheRequest(args.request(), aRequest, aBodyAction,
aSchemeAction, mStreamCleanupList, aRv);
aSchemeAction, aRv);
break;
}
case CacheOpArgs::TCacheKeysArgs: {
CacheKeysArgs& args = mOpArgs.get_CacheKeysArgs();
MOZ_DIAGNOSTIC_ASSERT(args.maybeRequest().isNothing());
args.maybeRequest().emplace(CacheRequest());
mTypeUtils->ToCacheRequest(args.maybeRequest().ref(), aRequest,
aBodyAction, aSchemeAction, mStreamCleanupList,
aRv);
aBodyAction, aSchemeAction, aRv);
break;
}
case CacheOpArgs::TStorageMatchArgs: {
StorageMatchArgs& args = mOpArgs.get_StorageMatchArgs();
mTypeUtils->ToCacheRequest(args.request(), aRequest, aBodyAction,
aSchemeAction, mStreamCleanupList, aRv);
aSchemeAction, aRv);
break;
}
default:
Expand Down Expand Up @@ -273,10 +262,6 @@ void AutoChildOpArgs::Add(JSContext* aCx, const InternalRequest& aRequest,
return;
}

// Ensure that we don't realloc the array since this can result
// in our AutoIPCStream objects to reference the wrong memory
// location. This should never happen and is a UAF if it does.
// Therefore make this a release assertion.
MOZ_RELEASE_ASSERT(args.requestResponseList().Length() <
args.requestResponseList().Capacity());

Expand All @@ -292,10 +277,9 @@ void AutoChildOpArgs::Add(JSContext* aCx, const InternalRequest& aRequest,
pair.response().body() = Nothing();

mTypeUtils->ToCacheRequest(pair.request(), aRequest, aBodyAction,
aSchemeAction, mStreamCleanupList, aRv);
aSchemeAction, aRv);
if (!aRv.Failed()) {
mTypeUtils->ToCacheResponse(aCx, pair.response(), aResponse,
mStreamCleanupList, aRv);
mTypeUtils->ToCacheResponse(aCx, pair.response(), aResponse, aRv);
}

if (aRv.Failed()) {
Expand All @@ -313,9 +297,6 @@ void AutoChildOpArgs::Add(JSContext* aCx, const InternalRequest& aRequest,
const CacheOpArgs& AutoChildOpArgs::SendAsOpArgs() {
MOZ_DIAGNOSTIC_ASSERT(!mSent);
mSent = true;
for (UniquePtr<AutoIPCStream>& autoStream : mStreamCleanupList) {
autoStream->TakeOptionalValue();
}
return mOpArgs;
}

Expand All @@ -330,12 +311,6 @@ AutoParentOpResult::AutoParentOpResult(
mSent(false) {
MOZ_DIAGNOSTIC_ASSERT(mManager);
MOZ_RELEASE_ASSERT(aEntryCount != 0);
// We are using AutoIPCStream objects to cleanup target IPCStream
// structures embedded in our CacheOpArgs. These IPCStream structs
// must not move once we attach our AutoIPCStream to them. Therefore,
// its important that any arrays containing streams are pre-sized for
// the number of entries we have in order to avoid realloc moving
// things around on us.
if (mOpResult.type() == CacheOpResult::TCacheMatchAllResult) {
CacheMatchAllResult& result = mOpResult.get_CacheMatchAllResult();
result.responseList().SetCapacity(aEntryCount);
Expand Down Expand Up @@ -370,8 +345,6 @@ AutoParentOpResult::~AutoParentOpResult() {
QM_WARNONLY_TRY(
OkIf(PCacheStreamControlParent::Send__delete__(mStreamControl)));
}

mStreamCleanupList.Clear();
}

void AutoParentOpResult::Add(CacheId aOpenedCacheId,
Expand Down Expand Up @@ -399,10 +372,6 @@ void AutoParentOpResult::Add(const SavedResponse& aSavedResponse,
}
case CacheOpResult::TCacheMatchAllResult: {
CacheMatchAllResult& result = mOpResult.get_CacheMatchAllResult();
// Ensure that we don't realloc the array since this can result
// in our AutoIPCStream objects to reference the wrong memory
// location. This should never happen and is a UAF if it does.
// Therefore make this a release assertion.
MOZ_RELEASE_ASSERT(result.responseList().Length() <
result.responseList().Capacity());
result.responseList().AppendElement(aSavedResponse.mValue);
Expand Down Expand Up @@ -430,10 +399,6 @@ void AutoParentOpResult::Add(const SavedRequest& aSavedRequest,
switch (mOpResult.type()) {
case CacheOpResult::TCacheKeysResult: {
CacheKeysResult& result = mOpResult.get_CacheKeysResult();
// Ensure that we don't realloc the array since this can result
// in our AutoIPCStream objects to reference the wrong memory
// location. This should never happen and is a UAF if it does.
// Therefore make this a release assertion.
MOZ_RELEASE_ASSERT(result.requestList().Length() <
result.requestList().Capacity());
result.requestList().AppendElement(aSavedRequest.mValue);
Expand All @@ -457,9 +422,6 @@ void AutoParentOpResult::Add(const SavedRequest& aSavedRequest,
const CacheOpResult& AutoParentOpResult::SendAsOpResult() {
MOZ_DIAGNOSTIC_ASSERT(!mSent);
mSent = true;
for (UniquePtr<AutoIPCStream>& autoStream : mStreamCleanupList) {
autoStream->TakeOptionalValue();
}
return mOpResult;
}

Expand Down Expand Up @@ -504,7 +466,7 @@ void AutoParentOpResult::SerializeReadStream(const nsID& aId,
RefPtr<ReadStream> readStream =
ReadStream::Create(mStreamControl, aId, stream);
ErrorResult rv;
readStream->Serialize(aReadStreamOut, mStreamCleanupList, rv);
readStream->Serialize(aReadStreamOut, rv);
MOZ_DIAGNOSTIC_ASSERT(!rv.Failed());
}

Expand Down
3 changes: 0 additions & 3 deletions dom/cache/AutoUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class ErrorResult;

namespace ipc {
class PBackgroundParent;
class AutoIPCStream;
} // namespace ipc

namespace dom {
Expand Down Expand Up @@ -65,7 +64,6 @@ class MOZ_STACK_CLASS AutoChildOpArgs final {
private:
TypeUtils* mTypeUtils;
CacheOpArgs mOpArgs;
nsTArray<UniquePtr<mozilla::ipc::AutoIPCStream>> mStreamCleanupList;
bool mSent;
};

Expand All @@ -92,7 +90,6 @@ class MOZ_STACK_CLASS AutoParentOpResult final {
mozilla::ipc::PBackgroundParent* mManager;
CacheOpResult mOpResult;
CacheStreamControlParent* mStreamControl;
nsTArray<UniquePtr<mozilla::ipc::AutoIPCStream>> mStreamCleanupList;
bool mSent;
};

Expand Down
12 changes: 4 additions & 8 deletions dom/cache/CacheStreamControlChild.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

namespace mozilla::dom::cache {

using mozilla::ipc::AutoIPCStream;
using mozilla::ipc::FileDescriptor;

// declared in ActorUtils.h
Expand Down Expand Up @@ -73,15 +72,12 @@ void CacheStreamControlChild::SerializeControl(
aReadStreamOut->controlChild() = this;
}

void CacheStreamControlChild::SerializeStream(
CacheReadStream* aReadStreamOut, nsIInputStream* aStream,
nsTArray<UniquePtr<AutoIPCStream>>& aStreamCleanupList) {
void CacheStreamControlChild::SerializeStream(CacheReadStream* aReadStreamOut,
nsIInputStream* aStream) {
NS_ASSERT_OWNINGTHREAD(CacheStreamControlChild);
MOZ_DIAGNOSTIC_ASSERT(aReadStreamOut);
UniquePtr<AutoIPCStream> autoStream(
new AutoIPCStream(aReadStreamOut->stream()));
autoStream->Serialize(aStream, Manager());
aStreamCleanupList.AppendElement(std::move(autoStream));
MOZ_ALWAYS_TRUE(mozilla::ipc::SerializeIPCStream(
do_AddRef(aStream), aReadStreamOut->stream(), /* aAllowLazy */ false));
}

void CacheStreamControlChild::OpenStream(const nsID& aId,
Expand Down
13 changes: 3 additions & 10 deletions dom/cache/CacheStreamControlChild.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@
#include "mozilla/dom/cache/StreamControl.h"
#include "nsTObserverArray.h"

namespace mozilla {
namespace ipc {
class AutoIPCStream;
} // namespace ipc
namespace dom::cache {
namespace mozilla::dom::cache {

class ReadStream;

Expand All @@ -35,9 +31,7 @@ class CacheStreamControlChild final : public PCacheStreamControlChild,
virtual void SerializeControl(CacheReadStream* aReadStreamOut) override;

virtual void SerializeStream(CacheReadStream* aReadStreamOut,
nsIInputStream* aStream,
nsTArray<UniquePtr<mozilla::ipc::AutoIPCStream>>&
aStreamCleanupList) override;
nsIInputStream* aStream) override;

virtual void OpenStream(const nsID& aId,
InputStreamResolver&& aResolver) override;
Expand All @@ -61,7 +55,6 @@ class CacheStreamControlChild final : public PCacheStreamControlChild,
bool mDestroyDelayed;
};

} // namespace dom::cache
} // namespace mozilla
} // namespace mozilla::dom::cache

#endif // mozilla_dom_cache_CacheStreamControlChild_h
21 changes: 9 additions & 12 deletions dom/cache/CacheStreamControlParent.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

namespace mozilla::dom::cache {

using mozilla::ipc::AutoIPCStream;
using mozilla::ipc::FileDescriptor;

// declared in ActorUtils.h
Expand All @@ -44,18 +43,14 @@ void CacheStreamControlParent::SerializeControl(
aReadStreamOut->controlParent() = this;
}

void CacheStreamControlParent::SerializeStream(
CacheReadStream* aReadStreamOut, nsIInputStream* aStream,
nsTArray<UniquePtr<AutoIPCStream>>& aStreamCleanupList) {
void CacheStreamControlParent::SerializeStream(CacheReadStream* aReadStreamOut,
nsIInputStream* aStream) {
NS_ASSERT_OWNINGTHREAD(CacheStreamControlParent);
MOZ_DIAGNOSTIC_ASSERT(aReadStreamOut);

UniquePtr<AutoIPCStream> autoStream(
new AutoIPCStream(aReadStreamOut->stream()));
DebugOnly<bool> ok = autoStream->Serialize(aStream, Manager());
DebugOnly<bool> ok = mozilla::ipc::SerializeIPCStream(
do_AddRef(aStream), aReadStreamOut->stream(), /* aAllowLazy */ false);
MOZ_ASSERT(ok);

aStreamCleanupList.AppendElement(std::move(autoStream));
}

void CacheStreamControlParent::OpenStream(const nsID& aId,
Expand Down Expand Up @@ -120,9 +115,11 @@ mozilla::ipc::IPCResult CacheStreamControlParent::RecvOpenStream(

OpenStream(aStreamId, [aResolver, self = RefPtr{this}](
nsCOMPtr<nsIInputStream>&& aStream) {
AutoIPCStream autoStream;
if (self->CanSend() && autoStream.Serialize(aStream, self->Manager())) {
aResolver(autoStream.TakeOptionalValue());
Maybe<IPCStream> stream;
if (self->CanSend() &&
mozilla::ipc::SerializeIPCStream(aStream.forget(), stream,
/* aAllowLazy */ false)) {
aResolver(stream);
} else {
aResolver(Nothing());
}
Expand Down
13 changes: 3 additions & 10 deletions dom/cache/CacheStreamControlParent.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@
#include "mozilla/dom/cache/StreamControl.h"
#include "nsTObserverArray.h"

namespace mozilla {
namespace ipc {
class AutoIPCStream;
} // namespace ipc
namespace dom::cache {
namespace mozilla::dom::cache {

class ReadStream;
class StreamList;
Expand All @@ -37,9 +33,7 @@ class CacheStreamControlParent final : public PCacheStreamControlParent,
virtual void SerializeControl(CacheReadStream* aReadStreamOut) override;

virtual void SerializeStream(CacheReadStream* aReadStreamOut,
nsIInputStream* aStream,
nsTArray<UniquePtr<mozilla::ipc::AutoIPCStream>>&
aStreamCleanupList) override;
nsIInputStream* aStream) override;

virtual void OpenStream(const nsID& aId,
InputStreamResolver&& aResolver) override;
Expand Down Expand Up @@ -73,7 +67,6 @@ class CacheStreamControlParent final : public PCacheStreamControlParent,
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CacheStreamControlParent, override)
};

} // namespace dom::cache
} // namespace mozilla
} // namespace mozilla::dom::cache

#endif // mozilla_dom_cache_CacheStreamControlParent_h
Loading

0 comments on commit 90aa76c

Please sign in to comment.