RGBCube/serenity
1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-07-23 20:17:42 +00:00
Code Issues Activity Actions

LibJS+LibWeb: Remove NonnullGCPtr<T>::operator=(GCPtr<T>) footgun

Browse source 
GCPtr can be null so it's not safe to assign it to a NonnullGCPtr unless
you know it to be non-null.

This exposed a number of wrong uses in LibWeb which had to be fixed as
part of this change.
This commit is contained in:
Andreas Kling 2022-12-14 13:43:57 +01:00
parent 4abdb68655
commit d5ed07fdc4
5 changed files with 32 additions and 39 deletions
Download patch file Download diff file Expand all files Collapse all files

7
Userland/Libraries/LibJS/Heap/GCPtr.h
View file

@ -49,13 +49,6 @@ public:
{ {
} }
NonnullGCPtr& operator=(GCPtr<T> const& other)
{
m_ptr = const_cast<T*>(other.ptr());
VERIFY(m_ptr);
return *this;
}
NonnullGCPtr& operator=(T const& other) NonnullGCPtr& operator=(T const& other)
{ {
m_ptr = &const_cast<T&>(other); m_ptr = &const_cast<T&>(other);

2
Userland/Libraries/LibWeb/DOM/EventDispatcher.cpp
View file

@ -285,7 +285,7 @@ bool EventDispatcher::dispatch(JS::NonnullGCPtr<EventTarget> target, Event& even
} }
// 8. Otherwise, set target to parent and then: // 8. Otherwise, set target to parent and then:
else { else {
target = parent; target = *parent;
// 1. If isActivationEvent is true, activationTarget is null, and target has activation behavior, then set activationTarget to target. // 1. If isActivationEvent is true, activationTarget is null, and target has activation behavior, then set activationTarget to target.
if (is_activation_event && !activation_target && target->activation_behavior) if (is_activation_event && !activation_target && target->activation_behavior)

10
Userland/Libraries/LibWeb/DOM/NodeIterator.cpp
View file

@ -84,7 +84,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> NodeIterator::traverse(Direction directio
auto* next_node = m_traversal_pointer->node->next_in_pre_order(m_root.ptr()); auto* next_node = m_traversal_pointer->node->next_in_pre_order(m_root.ptr());
if (!next_node) if (!next_node)
return nullptr; return nullptr;
m_traversal_pointer->node = next_node; m_traversal_pointer->node = *next_node;
} else { } else {
// If beforeNode is true, then set it to false. // If beforeNode is true, then set it to false.
m_traversal_pointer->is_before_node = false; m_traversal_pointer->is_before_node = false;
@ -99,7 +99,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> NodeIterator::traverse(Direction directio
auto* previous_node = m_traversal_pointer->node->previous_in_pre_order(); auto* previous_node = m_traversal_pointer->node->previous_in_pre_order();
if (!previous_node) if (!previous_node)
return nullptr; return nullptr;
m_traversal_pointer->node = previous_node; m_traversal_pointer->node = *previous_node;
} else { } else {
// If beforeNode is false, then set it to true. // If beforeNode is false, then set it to true.
m_traversal_pointer->is_before_node = true; m_traversal_pointer->is_before_node = true;
@ -194,7 +194,7 @@ void NodeIterator::run_pre_removing_steps_with_node_pointer(Node& to_be_removed_
while (node && node->is_descendant_of(to_be_removed_node)) while (node && node->is_descendant_of(to_be_removed_node))
node = node->next_in_pre_order(root()); node = node->next_in_pre_order(root());
if (node) if (node)
pointer.node = node; pointer.node = *node;
return; return;
} }
if (auto* node = to_be_removed_node.previous_in_pre_order()) { if (auto* node = to_be_removed_node.previous_in_pre_order()) {
@ -218,7 +218,7 @@ void NodeIterator::run_pre_removing_steps_with_node_pointer(Node& to_be_removed_
node = node->previous_in_pre_order(); node = node->previous_in_pre_order();
} }
if (node) if (node)
pointer.node = node; pointer.node = *node;
return; return;
} }
auto* node = to_be_removed_node.next_in_pre_order(root()); auto* node = to_be_removed_node.next_in_pre_order(root());
@ -227,7 +227,7 @@ void NodeIterator::run_pre_removing_steps_with_node_pointer(Node& to_be_removed_
node = node->previous_in_pre_order(); node = node->previous_in_pre_order();
} }
if (node) if (node)
pointer.node = node; pointer.node = *node;
} }
// https://dom.spec.whatwg.org/#nodeiterator-pre-removing-steps // https://dom.spec.whatwg.org/#nodeiterator-pre-removing-steps

26
Userland/Libraries/LibWeb/DOM/Range.cpp
View file

@ -116,7 +116,7 @@ RelativeBoundaryPointPosition position_of_boundary_point_relative_to_other_bound
while (!node_a.is_parent_of(child)) { while (!node_a.is_parent_of(child)) {
auto* parent = child->parent(); auto* parent = child->parent();
VERIFY(parent); VERIFY(parent);
child = parent; child = *parent;
} }
// 3. If child’s index is less than offsetA, then return after. // 3. If child’s index is less than offsetA, then return after.
@ -147,12 +147,12 @@ WebIDL::ExceptionOr<void> Range::set_start_or_end(Node& node, u32 offset, StartO
// 1. If range’s root is not equal to node’s root, or if bp is after the range’s end, set range’s end to bp. // 1. If range’s root is not equal to node’s root, or if bp is after the range’s end, set range’s end to bp.
if (&root() != &node.root() || position_of_boundary_point_relative_to_other_boundary_point(node, offset, m_end_container, m_end_offset) == RelativeBoundaryPointPosition::After) { if (&root() != &node.root() || position_of_boundary_point_relative_to_other_boundary_point(node, offset, m_end_container, m_end_offset) == RelativeBoundaryPointPosition::After) {
m_end_container = &node; m_end_container = node;
m_end_offset = offset; m_end_offset = offset;
} }
// 2. Set range’s start to bp. // 2. Set range’s start to bp.
m_start_container = &node; m_start_container = node;
m_start_offset = offset; m_start_offset = offset;
} else { } else {
// -> If these steps were invoked as "set the end" // -> If these steps were invoked as "set the end"
@ -160,12 +160,12 @@ WebIDL::ExceptionOr<void> Range::set_start_or_end(Node& node, u32 offset, StartO
// 1. If range’s root is not equal to node’s root, or if bp is before the range’s start, set range’s start to bp. // 1. If range’s root is not equal to node’s root, or if bp is before the range’s start, set range’s start to bp.
if (&root() != &node.root() || position_of_boundary_point_relative_to_other_boundary_point(node, offset, m_start_container, m_start_offset) == RelativeBoundaryPointPosition::Before) { if (&root() != &node.root() || position_of_boundary_point_relative_to_other_boundary_point(node, offset, m_start_container, m_start_offset) == RelativeBoundaryPointPosition::Before) {
m_start_container = &node; m_start_container = node;
m_start_offset = offset; m_start_offset = offset;
} }
// 2. Set range’s end to bp. // 2. Set range’s end to bp.
m_end_container = &node; m_end_container = node;
m_end_offset = offset; m_end_offset = offset;
} }
@ -342,11 +342,11 @@ WebIDL::ExceptionOr<void> Range::select(Node& node)
auto index = node.index(); auto index = node.index();
// 4. Set range’s start to boundary point (parent, index). // 4. Set range’s start to boundary point (parent, index).
m_start_container = parent; m_start_container = *parent;
m_start_offset = index; m_start_offset = index;
// 5. Set range’s end to boundary point (parent, index plus 1). // 5. Set range’s end to boundary point (parent, index plus 1).
m_end_container = parent; m_end_container = *parent;
m_end_offset = index + 1; m_end_offset = index + 1;
return {}; return {};
@ -384,11 +384,11 @@ WebIDL::ExceptionOr<void> Range::select_node_contents(Node const& node)
auto length = node.length(); auto length = node.length();
// 3. Set start to the boundary point (node, 0). // 3. Set start to the boundary point (node, 0).
m_start_container = &node; m_start_container = node;
m_start_offset = 0; m_start_offset = 0;
// 4. Set end to the boundary point (node, length). // 4. Set end to the boundary point (node, length).
m_end_container = &node; m_end_container = node;
m_end_offset = length; m_end_offset = length;
return {}; return {};
@ -428,7 +428,7 @@ JS::NonnullGCPtr<Node> Range::common_ancestor_container() const
// 2. While container is not an inclusive ancestor of end node, let container be container’s parent. // 2. While container is not an inclusive ancestor of end node, let container be container’s parent.
while (!container->is_inclusive_ancestor_of(m_end_container)) { while (!container->is_inclusive_ancestor_of(m_end_container)) {
VERIFY(container->parent()); VERIFY(container->parent());
container = container->parent(); container = *container->parent();
} }
// 3. Return container. // 3. Return container.
@ -593,7 +593,7 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<DocumentFragment>> Range::extract()
// 6. While common ancestor is not an inclusive ancestor of original end node, set common ancestor to its own parent. // 6. While common ancestor is not an inclusive ancestor of original end node, set common ancestor to its own parent.
while (!common_ancestor->is_inclusive_ancestor_of(original_end_node)) while (!common_ancestor->is_inclusive_ancestor_of(original_end_node))
common_ancestor = common_ancestor->parent_node(); common_ancestor = *common_ancestor->parent_node();
// 7. Let first partially contained child be null. // 7. Let first partially contained child be null.
JS::GCPtr<Node> first_partially_contained_child; JS::GCPtr<Node> first_partially_contained_child;
@ -919,7 +919,7 @@ WebIDL::ExceptionOr<JS::NonnullGCPtr<DocumentFragment>> Range::clone_the_content
// 6. While common ancestor is not an inclusive ancestor of original end node, set common ancestor to its own parent. // 6. While common ancestor is not an inclusive ancestor of original end node, set common ancestor to its own parent.
while (!common_ancestor->is_inclusive_ancestor_of(original_end_node)) while (!common_ancestor->is_inclusive_ancestor_of(original_end_node))
common_ancestor = common_ancestor->parent_node(); common_ancestor = *common_ancestor->parent_node();
// 7. Let first partially contained child be null. // 7. Let first partially contained child be null.
JS::GCPtr<Node> first_partially_contained_child; JS::GCPtr<Node> first_partially_contained_child;
@ -1078,7 +1078,7 @@ WebIDL::ExceptionOr<void> Range::delete_contents()
// 2. While reference node’s parent is not null and is not an inclusive ancestor of original end node, set reference node to its parent. // 2. While reference node’s parent is not null and is not an inclusive ancestor of original end node, set reference node to its parent.
while (reference_node->parent_node() && !reference_node->parent_node()->is_inclusive_ancestor_of(original_end_node)) while (reference_node->parent_node() && !reference_node->parent_node()->is_inclusive_ancestor_of(original_end_node))
reference_node = reference_node->parent_node(); reference_node = *reference_node->parent_node();
// 3. Set new node to the parent of reference node, and new offset to one plus the index of reference node. // 3. Set new node to the parent of reference node, and new offset to one plus the index of reference node.
new_node = reference_node->parent_node(); new_node = reference_node->parent_node();

26
Userland/Libraries/LibWeb/DOM/TreeWalker.cpp
View file

@ -57,7 +57,7 @@ JS::NonnullGCPtr<Node> TreeWalker::current_node() const
// https://dom.spec.whatwg.org/#dom-treewalker-currentnode // https://dom.spec.whatwg.org/#dom-treewalker-currentnode
void TreeWalker::set_current_node(Node& node) void TreeWalker::set_current_node(Node& node)
{ {
m_current = &node; m_current = node;
} }
// https://dom.spec.whatwg.org/#dom-treewalker-parentnode // https://dom.spec.whatwg.org/#dom-treewalker-parentnode
@ -76,7 +76,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::parent_node()
if (node) { if (node) {
auto result = TRY(filter(*node)); auto result = TRY(filter(*node));
if (result == NodeFilter::FILTER_ACCEPT) { if (result == NodeFilter::FILTER_ACCEPT) {
m_current = node; m_current = *node;
return node; return node;
} }
} }
@ -113,7 +113,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_sibling()
JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node() JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node()
{ {
// 1. Let node be this’s current. // 1. Let node be this’s current.
JS::GCPtr<Node> node = m_current; JS::NonnullGCPtr<Node> node = m_current;
// 2. While node is not this’s root: // 2. While node is not this’s root:
while (node != m_root) { while (node != m_root) {
@ -123,7 +123,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node()
// 2. While sibling is non-null: // 2. While sibling is non-null:
while (sibling) { while (sibling) {
// 1. Set node to sibling. // 1. Set node to sibling.
node = sibling; node = *sibling;
// 2. Let result be the result of filtering node within this. // 2. Let result be the result of filtering node within this.
auto result = TRY(filter(*node)); auto result = TRY(filter(*node));
@ -131,7 +131,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node()
// 3. While result is not FILTER_REJECT and node has a child: // 3. While result is not FILTER_REJECT and node has a child:
while (result != NodeFilter::FILTER_REJECT && node->has_children()) { while (result != NodeFilter::FILTER_REJECT && node->has_children()) {
// 1. Set node to node’s last child. // 1. Set node to node’s last child.
node = node->last_child(); node = *node->last_child();
// 2. Set result to the result of filtering node within this. // 2. Set result to the result of filtering node within this.
result = TRY(filter(*node)); result = TRY(filter(*node));
@ -152,7 +152,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node()
return nullptr; return nullptr;
// 4. Set node to node’s parent. // 4. Set node to node’s parent.
node = node->parent(); node = *node->parent();
// 5. If the return value of filtering node within this is FILTER_ACCEPT, then set this’s current to node and return node. // 5. If the return value of filtering node within this is FILTER_ACCEPT, then set this’s current to node and return node.
if (TRY(filter(*node)) == NodeFilter::FILTER_ACCEPT) { if (TRY(filter(*node)) == NodeFilter::FILTER_ACCEPT) {
@ -168,7 +168,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::previous_node()
JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_node() JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_node()
{ {
// 1. Let node be this’s current. // 1. Let node be this’s current.
JS::GCPtr<Node> node = m_current; JS::NonnullGCPtr<Node> node = m_current;
// 2. Let result be FILTER_ACCEPT. // 2. Let result be FILTER_ACCEPT.
auto result = NodeFilter::FILTER_ACCEPT; auto result = NodeFilter::FILTER_ACCEPT;
@ -178,14 +178,14 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_node()
// 1. While result is not FILTER_REJECT and node has a child: // 1. While result is not FILTER_REJECT and node has a child:
while (result != NodeFilter::FILTER_REJECT && node->has_children()) { while (result != NodeFilter::FILTER_REJECT && node->has_children()) {
// 1. Set node to its first child. // 1. Set node to its first child.
node = node->first_child(); node = *node->first_child();
// 2. Set result to the result of filtering node within this. // 2. Set result to the result of filtering node within this.
auto result = TRY(filter(*node)); auto result = TRY(filter(*node));
// 3. If result is FILTER_ACCEPT, then set this’s current to node and return node. // 3. If result is FILTER_ACCEPT, then set this’s current to node and return node.
if (result == NodeFilter::FILTER_ACCEPT) { if (result == NodeFilter::FILTER_ACCEPT) {
m_current = node; m_current = *node;
return node; return node;
} }
} }
@ -207,7 +207,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_node()
// 3. If sibling is non-null, then set node to sibling and break. // 3. If sibling is non-null, then set node to sibling and break.
if (sibling) { if (sibling) {
node = sibling; node = *sibling;
break; break;
} }
@ -220,7 +220,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::next_node()
// 6. If result is FILTER_ACCEPT, then set this’s current to node and return node. // 6. If result is FILTER_ACCEPT, then set this’s current to node and return node.
if (result == NodeFilter::FILTER_ACCEPT) { if (result == NodeFilter::FILTER_ACCEPT) {
m_current = node; m_current = *node;
return node; return node;
} }
} }
@ -279,7 +279,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::traverse_children(ChildTraver
// 2. If result is FILTER_ACCEPT, then set walker’s current to node and return node. // 2. If result is FILTER_ACCEPT, then set walker’s current to node and return node.
if (result == NodeFilter::FILTER_ACCEPT) { if (result == NodeFilter::FILTER_ACCEPT) {
m_current = node; m_current = *node;
return node; return node;
} }
@ -347,7 +347,7 @@ JS::ThrowCompletionOr<JS::GCPtr<Node>> TreeWalker::traverse_siblings(SiblingTrav
// 3. If result is FILTER_ACCEPT, then set walker’s current to node and return node. // 3. If result is FILTER_ACCEPT, then set walker’s current to node and return node.
if (result == NodeFilter::FILTER_ACCEPT) { if (result == NodeFilter::FILTER_ACCEPT) {
m_current = node; m_current = *node;
return node; return node;
} }