From 7fae46361b0303488d954a409f10aef331d7f78b Mon Sep 17 00:00:00 2001
From: Luke Wilde <lukew@serenityos.org>
Date: Wed, 15 Dec 2021 09:08:55 +0000
Subject: [PATCH] LibWeb: Fix null-deref in <table> delete_row with index = -1
 and no rows
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This wasn't quite following what the spec says for step 2:
"If index is −1, then remove the last element in the rows collection
from its parent, or do nothing if the rows collection is empty."

It was behaving like:
"If index is −1 and the rows collection is not empty, then remove the
last element in the rows collection from its parent."

Which is not the same, as it will fall into the "Otherwise" if
`index == -1` and the rows collection is empty and try and get the -2nd
element of the rows.

Found with Domato.
---
 .../LibWeb/HTML/HTMLTableElement.cpp          | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp
index f84057cbec..de245b2e81 100644
--- a/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp
+++ b/Userland/Libraries/LibWeb/HTML/HTMLTableElement.cpp
@@ -304,22 +304,29 @@ DOM::ExceptionOr<NonnullRefPtr<HTMLTableRowElement>> HTMLTableElement::insert_ro
     return tr;
 }
 
+// https://html.spec.whatwg.org/multipage/tables.html#dom-table-deleterow
 DOM::ExceptionOr<void> HTMLTableElement::delete_row(long index)
 {
     auto rows = this->rows();
     auto rows_length = rows->length();
 
-    if (index < -1 || index >= (long)rows_length) {
+    // 1. If index is less than −1 or greater than or equal to the number of elements in the rows collection, then throw an "IndexSizeError" DOMException.
+    if (index < -1 || index >= (long)rows_length)
         return DOM::IndexSizeError::create("Index is negative or greater than the number of rows");
-    }
-    if (index == -1 && rows_length > 0) {
+
+    // 2. If index is −1, then remove the last element in the rows collection from its parent, or do nothing if the rows collection is empty.
+    if (index == -1) {
+        if (rows_length == 0)
+            return {};
+
         auto row_to_remove = rows->item(rows_length - 1);
         row_to_remove->remove(false);
-    } else {
-        auto row_to_remove = rows->item(index);
-        row_to_remove->remove(false);
+        return {};
     }
 
+    // 3. Otherwise, remove the indexth element in the rows collection from its parent.
+    auto row_to_remove = rows->item(index);
+    row_to_remove->remove(false);
     return {};
 }