From 8d88df10f6e02f246148daaa14bd9f275079c928 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Wed, 2 Feb 2022 00:48:19 +0000 Subject: [PATCH] Bug 1718825 - Treat strong pointers which are members of class/struct and marked as `MOZ_KNOWN_LIVE` as safe r=andi `MOZ_KNOWN_LIVE RefPtr mFoo` is not treated as safe because its raw pointer is referred with operators but they are not checked at handling `MOZ_KNOWN_LIVE` annotation. Additionally, when members marked as `MOZ_KNOWN_LIVE` are in the stack, they are also not treated as safe, but they should be safe in most cases. With these changes, `HTMLTableEditor.cpp` can get rid of a lot of `MOZ_KnownLive` method calls. Differential Revision: https://phabricator.services.mozilla.com/D136122 --- build/clang-plugin/CanRunScriptChecker.cpp | 18 +++-- build/clang-plugin/tests/TestCanRunScript.cpp | 45 +++++++++++-- editor/libeditor/HTMLTableEditor.cpp | 67 ++++++++----------- 3 files changed, 80 insertions(+), 50 deletions(-) diff --git a/build/clang-plugin/CanRunScriptChecker.cpp b/build/clang-plugin/CanRunScriptChecker.cpp index f3f3dbb50684..f68c32b02ad6 100644 --- a/build/clang-plugin/CanRunScriptChecker.cpp +++ b/build/clang-plugin/CanRunScriptChecker.cpp @@ -84,7 +84,9 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) { auto KnownLiveMemberOfParam = memberExpr(hasKnownLiveAnnotation(), - hasObjectExpression(ignoreTrivials(KnownLiveParam))); + hasObjectExpression(anyOf( + ignoreTrivials(KnownLiveParam), + declRefExpr(to(varDecl(hasAutomaticStorageDuration())))))); // A matcher that matches various things that are known to be live directly, // without making any assumptions about operators. @@ -111,11 +113,17 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) { // example). For purposes of this analysis we are assuming the method // calls on smart ptrs all just return the pointer inside, cxxMemberCallExpr( - on(allOf(hasType(isSmartPtrToRefCounted()), KnownLiveBase))), + on(anyOf(allOf(hasType(isSmartPtrToRefCounted()), KnownLiveBase), + // Allow it if calling a member method which is marked as + // MOZ_KNOWN_LIVE + KnownLiveMemberOfParam))), // operator* or operator-> on a thing that is already known to be live. - cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("*"), - hasOverloadedOperatorName("->")), - hasAnyArgument(KnownLiveBase), argumentCountIs(1)), + cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("*"), + hasOverloadedOperatorName("->")), + hasAnyArgument( + anyOf(KnownLiveBase, ignoreTrivials(KnownLiveMemberOfParam))), + argumentCountIs(1)), // A dereference on a thing that is known to be live. This is _not_ // caught by the "operator* or operator->" clause above, because // cxxOperatorCallExpr() only catches cases when a class defines diff --git a/build/clang-plugin/tests/TestCanRunScript.cpp b/build/clang-plugin/tests/TestCanRunScript.cpp index d47595ca2dfb..7d923674e61c 100644 --- a/build/clang-plugin/tests/TestCanRunScript.cpp +++ b/build/clang-plugin/tests/TestCanRunScript.cpp @@ -623,33 +623,64 @@ struct DisallowMemberArgsViaReferenceAlias2 { struct AllowMozKnownLiveMember { public: MOZ_KNOWN_LIVE RefCountedBase* mWhatever; - MOZ_CAN_RUN_SCRIPT void foo(RefCountedBase* aWhatever) {} + MOZ_KNOWN_LIVE RefPtr mRefCountedWhatever; + MOZ_CAN_RUN_SCRIPT void fooPtr(RefCountedBase* aWhatever) {} + MOZ_CAN_RUN_SCRIPT void fooRef(RefCountedBase& aWhatever) {} MOZ_CAN_RUN_SCRIPT void bar() { - foo(mWhatever); + fooPtr(mWhatever); + fooRef(*mWhatever); + fooPtr(mRefCountedWhatever); + fooRef(*mRefCountedWhatever); } }; struct AllowMozKnownLiveMemberParent : AllowMozKnownLiveMember { MOZ_CAN_RUN_SCRIPT void baz() { - foo(mWhatever); + fooPtr(mWhatever); + fooRef(*mWhatever); + fooPtr(mRefCountedWhatever); + fooRef(*mRefCountedWhatever); } }; struct AllowMozKnownLiveParamMember { public: MOZ_CAN_RUN_SCRIPT void foo(AllowMozKnownLiveMember& aAllow) { - aAllow.foo(aAllow.mWhatever); + aAllow.fooPtr(aAllow.mWhatever); + aAllow.fooRef(*aAllow.mWhatever); + aAllow.fooPtr(aAllow.mRefCountedWhatever); + aAllow.fooRef(*aAllow.mRefCountedWhatever); } MOZ_CAN_RUN_SCRIPT void bar(AllowMozKnownLiveMemberParent& aAllowParent) { - aAllowParent.foo(aAllowParent.mWhatever); + aAllowParent.fooPtr(aAllowParent.mWhatever); + aAllowParent.fooRef(*aAllowParent.mWhatever); + aAllowParent.fooPtr(aAllowParent.mRefCountedWhatever); + aAllowParent.fooRef(*aAllowParent.mRefCountedWhatever); } }; +MOZ_CAN_RUN_SCRIPT void AllowMozKnownLiveMemberInAutoStorage() { + AllowMozKnownLiveMember inStack; + AllowMozKnownLiveMember* inHeap = new AllowMozKnownLiveMember(); + inStack.fooPtr(inStack.mWhatever); + inStack.fooRef(*inStack.mWhatever); + inStack.fooPtr(inStack.mRefCountedWhatever); + inStack.fooRef(*inStack.mRefCountedWhatever); + inStack.fooPtr(inHeap->mWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'inHeap->mWhatever' is neither.}} + inStack.fooRef(*inHeap->mWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). '*inHeap->mWhatever' is neither.}} + inStack.fooPtr(inHeap->mRefCountedWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'inHeap->mRefCountedWhatever' is neither.}} + inStack.fooRef(*inHeap->mRefCountedWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). '*inHeap->mRefCountedWhatever' is neither.}} +} + struct DisallowMozKnownLiveMemberNotFromKnownLive { AllowMozKnownLiveMember* mMember; - MOZ_CAN_RUN_SCRIPT void foo(RefCountedBase* aWhatever) {} + MOZ_CAN_RUN_SCRIPT void fooPtr(RefCountedBase* aWhatever) {} + MOZ_CAN_RUN_SCRIPT void fooRef(RefCountedBase& aWhatever) {} MOZ_CAN_RUN_SCRIPT void bar() { - foo(mMember->mWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'mMember->mWhatever' is neither.}} + fooPtr(mMember->mWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'mMember->mWhatever' is neither.}} + fooRef(*mMember->mWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). '*mMember->mWhatever' is neither.}} + fooPtr(mMember->mRefCountedWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'mMember->mRefCountedWhatever' is neither.}} + fooRef(*mMember->mRefCountedWhatever); // expected-error {{arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). '*mMember->mRefCountedWhatever' is neither.}} } }; diff --git a/editor/libeditor/HTMLTableEditor.cpp b/editor/libeditor/HTMLTableEditor.cpp index 96063e7def89..6ba8be812320 100644 --- a/editor/libeditor/HTMLTableEditor.cpp +++ b/editor/libeditor/HTMLTableEditor.cpp @@ -49,8 +49,8 @@ using EmptyCheckOption = HTMLEditUtils::EmptyCheckOption; */ class MOZ_STACK_CLASS AutoSelectionSetterAfterTableEdit final { private: - RefPtr mHTMLEditor; - RefPtr mTable; + MOZ_KNOWN_LIVE RefPtr mHTMLEditor; + MOZ_KNOWN_LIVE RefPtr mTable; int32_t mCol, mRow, mDirection, mSelected; public: @@ -66,9 +66,8 @@ class MOZ_STACK_CLASS AutoSelectionSetterAfterTableEdit final { MOZ_CAN_RUN_SCRIPT ~AutoSelectionSetterAfterTableEdit() { if (mHTMLEditor) { - MOZ_KnownLive(mHTMLEditor) - ->SetSelectionAfterTableEdit(MOZ_KnownLive(mTable), mRow, mCol, - mDirection, mSelected); + mHTMLEditor->SetSelectionAfterTableEdit(mTable, mRow, mCol, mDirection, + mSelected); } } @@ -511,7 +510,7 @@ nsresult HTMLEditor::InsertTableColumnsWithTransaction( // Thus we set the colspan to its true value. if (!cellDataAtSelection.mColSpan) { DebugOnly rvIgnored = - SetColSpan(MOZ_KnownLive(cellDataAtSelection.mElement), + SetColSpan(cellDataAtSelection.mElement, cellDataAtSelection.mEffectiveColSpan); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::SetColSpan() failed, but ignored"); @@ -565,9 +564,8 @@ nsresult HTMLEditor::InsertTableColumnsWithTransaction( // Note: we do nothing if colsspan=0, since it should automatically // span the new column. if (cellData.mColSpan > 0) { - DebugOnly rvIgnored = - SetColSpan(MOZ_KnownLive(cellData.mElement), - cellData.mColSpan + aNumberOfColumnsToInsert); + DebugOnly rvIgnored = SetColSpan( + cellData.mElement, cellData.mColSpan + aNumberOfColumnsToInsert); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::SetColSpan() failed, but ignored"); } @@ -577,8 +575,7 @@ nsresult HTMLEditor::InsertTableColumnsWithTransaction( // Simply set selection to the current cell. So, we can let // InsertTableCellsWithTransaction() do the work. Insert a new cell // before current one. - CollapseSelectionToStartOf(MOZ_KnownLive(*cellData.mElement), - ignoredError); + CollapseSelectionToStartOf(*cellData.mElement, ignoredError); if (NS_WARN_IF(ignoredError.ErrorCodeIs(NS_ERROR_EDITOR_DESTROYED))) { return NS_ERROR_EDITOR_DESTROYED; } @@ -745,7 +742,7 @@ nsresult HTMLEditor::InsertTableRowsWithTransaction( // Thus we set the rowspan to its true value. if (!cellDataAtSelection.mRowSpan) { DebugOnly rvIgnored = - SetRowSpan(MOZ_KnownLive(cellDataAtSelection.mElement), + SetRowSpan(cellDataAtSelection.mElement, cellDataAtSelection.mEffectiveRowSpan); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::SetRowSpan() failed, but ignored"); @@ -786,9 +783,8 @@ nsresult HTMLEditor::InsertTableRowsWithTransaction( // Note that if rowspan is 0, we do nothing since that cell should // automatically extend into the new row. if (cellData.mRowSpan > 0) { - DebugOnly rvIgnored = - SetRowSpan(MOZ_KnownLive(cellData.mElement), - cellData.mRowSpan + aNumberOfRowsToInsert); + DebugOnly rvIgnored = SetRowSpan( + cellData.mElement, cellData.mRowSpan + aNumberOfRowsToInsert); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::SetRowSpan() failed, but ignored"); } @@ -1549,7 +1545,7 @@ nsresult HTMLEditor::DeleteTableColumnWithTransaction(Element& aTableElement, NS_WARNING_ASSERTION(cellData.mColSpan > 1, "colspan should be 2 or larger"); DebugOnly rvIgnored = - SetColSpan(MOZ_KnownLive(cellData.mElement), cellData.mColSpan - 1); + SetColSpan(cellData.mElement, cellData.mColSpan - 1); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::SetColSpan() failed, but ignored"); } @@ -1558,7 +1554,7 @@ nsresult HTMLEditor::DeleteTableColumnWithTransaction(Element& aTableElement, // so delete contents of cell instead of cell itself (We must have // reset colspan above). DebugOnly rvIgnored = - DeleteAllChildrenWithTransaction(MOZ_KnownLive(*cellData.mElement)); + DeleteAllChildrenWithTransaction(*cellData.mElement); NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored), "HTMLEditor::DeleteAllChildrenWithTransaction() " "failed, but ignored"); @@ -1577,8 +1573,7 @@ nsresult HTMLEditor::DeleteTableColumnWithTransaction(Element& aTableElement, if (numberOfCellsInRow != 1) { // If removing cell is not the last cell of the row, we can just remove // it. - nsresult rv = - DeleteNodeWithTransaction(MOZ_KnownLive(*cellData.mElement)); + nsresult rv = DeleteNodeWithTransaction(*cellData.mElement); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::DeleteNodeWithTransaction() failed"); return rv; @@ -2044,8 +2039,7 @@ NS_IMETHODIMP HTMLEditor::SelectAllTableCells() { // XXX So, we should distinguish whether CellData returns error or just // not found later. if (cellData.mElement && !cellData.IsSpannedFromOtherRowOrColumn()) { - nsresult rv = - AppendContentToSelectionAsRange(MOZ_KnownLive(*cellData.mElement)); + nsresult rv = AppendContentToSelectionAsRange(*cellData.mElement); if (rv == NS_ERROR_EDITOR_DESTROYED) { NS_WARNING( "HTMLEditor::AppendContentToSelectionAsRange() caused destroying " @@ -2159,7 +2153,7 @@ NS_IMETHODIMP HTMLEditor::SelectTableRow() { // XXX So, we should distinguish whether CellData returns error or just // not found later. if (cellData.mElement && !cellData.IsSpannedFromOtherRowOrColumn()) { - rv = AppendContentToSelectionAsRange(MOZ_KnownLive(*cellData.mElement)); + rv = AppendContentToSelectionAsRange(*cellData.mElement); if (rv == NS_ERROR_EDITOR_DESTROYED) { NS_WARNING( "HTMLEditor::AppendContentToSelectionAsRange() caused destroying " @@ -2268,7 +2262,7 @@ NS_IMETHODIMP HTMLEditor::SelectTableColumn() { // XXX So, we should distinguish whether CellData returns error or just // not found later. if (cellData.mElement && !cellData.IsSpannedFromOtherRowOrColumn()) { - rv = AppendContentToSelectionAsRange(MOZ_KnownLive(*cellData.mElement)); + rv = AppendContentToSelectionAsRange(*cellData.mElement); if (rv == NS_ERROR_EDITOR_DESTROYED) { NS_WARNING( "HTMLEditor::AppendContentToSelectionAsRange() caused destroying " @@ -2447,7 +2441,7 @@ nsresult HTMLEditor::SplitCellIntoColumns(Element* aTable, int32_t aRowIndex, } // Reduce colspan of cell to split - nsresult rv = SetColSpan(MOZ_KnownLive(cellData.mElement), aColSpanLeft); + nsresult rv = SetColSpan(cellData.mElement, aColSpanLeft); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetColSpan() failed"); return rv; @@ -2456,8 +2450,8 @@ nsresult HTMLEditor::SplitCellIntoColumns(Element* aTable, int32_t aRowIndex, // Insert new cell after using the remaining span // and always get the new cell so we can copy the background color; RefPtr newCellElement; - rv = InsertCell(MOZ_KnownLive(cellData.mElement), cellData.mEffectiveRowSpan, - aColSpanRight, true, false, getter_AddRefs(newCellElement)); + rv = InsertCell(cellData.mElement, cellData.mEffectiveRowSpan, aColSpanRight, + true, false, getter_AddRefs(newCellElement)); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::InsertCell() failed"); return rv; @@ -2468,8 +2462,7 @@ nsresult HTMLEditor::SplitCellIntoColumns(Element* aTable, int32_t aRowIndex, if (aNewCell) { *aNewCell = do_AddRef(newCellElement).take(); } - rv = - CopyCellBackgroundColor(newCellElement, MOZ_KnownLive(cellData.mElement)); + rv = CopyCellBackgroundColor(newCellElement, cellData.mElement); NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "HTMLEditor::CopyCellBackgroundColor() failed"); return rv; @@ -2573,7 +2566,7 @@ nsresult HTMLEditor::SplitCellIntoRows(Element* aTable, int32_t aRowIndex, } // Reduce rowspan of cell to split - nsresult rv = SetRowSpan(MOZ_KnownLive(cellData.mElement), aRowSpanAbove); + nsresult rv = SetRowSpan(cellData.mElement, aRowSpanAbove); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetRowSpan() failed"); return rv; @@ -3047,8 +3040,7 @@ NS_IMETHODIMP HTMLEditor::JoinTableCells(bool aMergeNonContiguousContents) { if (spanAboveMergedCell > 0) { // Cell we merged started in a row above the target cell // Reduce rowspan to give room where target cell will extend its colspan - nsresult rv = SetRowSpan(MOZ_KnownLive(rightCellData.mElement), - spanAboveMergedCell); + nsresult rv = SetRowSpan(rightCellData.mElement, spanAboveMergedCell); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetRowSpan() failed"); return EditorBase::ToGenericNSResult(rv); @@ -3056,9 +3048,8 @@ NS_IMETHODIMP HTMLEditor::JoinTableCells(bool aMergeNonContiguousContents) { } // Reset target cell's colspan to encompass cell to the right - rv = SetColSpan( - MOZ_KnownLive(leftCellData.mElement), - leftCellData.mEffectiveColSpan + rightCellData.mEffectiveColSpan); + rv = SetColSpan(leftCellData.mElement, leftCellData.mEffectiveColSpan + + rightCellData.mEffectiveColSpan); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetColSpan() failed"); return EditorBase::ToGenericNSResult(rv); @@ -3205,8 +3196,8 @@ nsresult HTMLEditor::FixBadRowSpan(Element* aTable, int32_t aRowIndex, // not found a cell. Fix this later. if (cellData.mElement && cellData.mRowSpan > 0 && !cellData.IsSpannedFromOtherRowOrColumn()) { - nsresult rv = SetRowSpan(MOZ_KnownLive(cellData.mElement), - cellData.mRowSpan - rowsReduced); + nsresult rv = + SetRowSpan(cellData.mElement, cellData.mRowSpan - rowsReduced); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetRawSpan() failed"); return rv; @@ -3285,8 +3276,8 @@ nsresult HTMLEditor::FixBadColSpan(Element* aTable, int32_t aColIndex, // not found a cell. Fix this later. if (cellData.mElement && cellData.mColSpan > 0 && !cellData.IsSpannedFromOtherRowOrColumn()) { - nsresult rv = SetColSpan(MOZ_KnownLive(cellData.mElement), - cellData.mColSpan - colsReduced); + nsresult rv = + SetColSpan(cellData.mElement, cellData.mColSpan - colsReduced); if (NS_FAILED(rv)) { NS_WARNING("HTMLEditor::SetColSpan() failed"); return rv;