eval: Refactor some potentially dangerous list appends

src/nvim/eval.c

@@ -2421,9 +2421,11 @@ static void set_var_lval(lval_T *lp, char_u *endp, typval_T *rettv,
       if (TV_LIST_ITEM_NEXT(lp->ll_list, lp->ll_li) == NULL) {
         // Need to add an empty item.
         tv_list_append_number(lp->ll_list, 0);
-        assert(TV_LIST_ITEM_NEXT(lp->ll_list, lp->ll_li));
+        // ll_li may have become invalid after append, don’t use it.
-      }
+        lp->ll_li = tv_list_last(lp->ll_list);  // Valid again.
+      } else {
         lp->ll_li = TV_LIST_ITEM_NEXT(lp->ll_list, lp->ll_li);
+      }
       lp->ll_n1++;
     }
     if (ri != NULL) {
@@ -4528,7 +4530,7 @@ eval_index (
           item = tv_list_find(rettv->vval.v_list, n1);
           while (n1++ <= n2) {
             tv_list_append_tv(l, TV_LIST_ITEM_TV(item));
-            item = TV_LIST_ITEM_NEXT(l, item);
+            item = TV_LIST_ITEM_NEXT(rettv->vval.v_list, item);
           }
           tv_clear(rettv);
           rettv->v_type = VAR_LIST;
@@ -12529,12 +12531,12 @@ static void f_matcharg(typval_T *argvars, typval_T *rettv, FunPtr fptr)
 {
   tv_list_alloc_ret(rettv);
 
-  int id = tv_get_number(&argvars[0]);
+  const int id = tv_get_number(&argvars[0]);
 
   if (id >= 1 && id <= 3) {
-    matchitem_T *m;
+    matchitem_T *const m = (matchitem_T *)get_match(curwin, id);
 
-    if ((m = (matchitem_T *)get_match(curwin, id)) != NULL) {
+    if (m != NULL) {
       tv_list_append_string(rettv->vval.v_list,
                             (const char *)syn_id2name(m->hlg_id), -1);
       tv_list_append_string(rettv->vval.v_list, (const char *)m->pattern, -1);
@@ -15135,12 +15137,6 @@ static void f_sockconnect(typval_T *argvars, typval_T *rettv, FunPtr fptr)
   rettv->v_type = VAR_NUMBER;
 }
 
-/// struct used in the array that's given to qsort()
-typedef struct {
-  listitem_T *item;
-  int idx;
-} sortItem_T;
-
 /// struct storing information about current sort
 typedef struct {
   int item_compare_ic;
@@ -15161,8 +15157,8 @@ static sortinfo_T *sortinfo = NULL;
  */
 static int item_compare(const void *s1, const void *s2, bool keep_zero)
 {
-  sortItem_T *const si1 = (sortItem_T *)s1;
+  ListSortItem *const si1 = (ListSortItem *)s1;
-  sortItem_T *const si2 = (sortItem_T *)s2;
+  ListSortItem *const si2 = (ListSortItem *)s2;
 
   typval_T *const tv1 = TV_LIST_ITEM_TV(si1->item);
   typval_T *const tv2 = TV_LIST_ITEM_TV(si2->item);
@@ -15256,7 +15252,7 @@ static int item_compare_not_keeping_zero(const void *s1, const void *s2)
 
 static int item_compare2(const void *s1, const void *s2, bool keep_zero)
 {
-  sortItem_T  *si1, *si2;
+  ListSortItem *si1, *si2;
   int res;
   typval_T rettv;
   typval_T argv[3];
@@ -15269,8 +15265,8 @@ static int item_compare2(const void *s1, const void *s2, bool keep_zero)
     return 0;
   }
 
-  si1 = (sortItem_T *)s1;
+  si1 = (ListSortItem *)s1;
-  si2 = (sortItem_T *)s2;
+  si2 = (ListSortItem *)s2;
 
   if (partial == NULL) {
     func_name = sortinfo->item_compare_func;
@@ -15327,7 +15323,7 @@ static int item_compare2_not_keeping_zero(const void *s1, const void *s2)
  */
 static void do_sort_uniq(typval_T *argvars, typval_T *rettv, bool sort)
 {
-  sortItem_T  *ptrs;
+  ListSortItem  *ptrs;
   long len;
   long i;
 
@@ -15417,42 +15413,21 @@ static void do_sort_uniq(typval_T *argvars, typval_T *rettv, bool sort)
     }
 
     /* Make an array with each entry pointing to an item in the List. */
-    ptrs = xmalloc((size_t)(len * sizeof (sortItem_T)));
+    ptrs = xmalloc((size_t)(len * sizeof(ListSortItem)));
 
-    i = 0;
     if (sort) {
-      // sort(): ptrs will be the list to sort.
-      TV_LIST_ITER(l, li, {
-        ptrs[i].item = li;
-        ptrs[i].idx = i;
-        i++;
-      });
-
       info.item_compare_func_err = false;
-      // Test the compare function.
+      tv_list_item_sort(l, ptrs,
-      if ((info.item_compare_func != NULL
+                        ((info.item_compare_func == NULL
-           || info.item_compare_partial != NULL)
+                          && info.item_compare_partial == NULL)
-          && item_compare2_not_keeping_zero(&ptrs[0], &ptrs[1])
+                         ? item_compare_not_keeping_zero
-             == ITEM_COMPARE_FAIL) {
+                         : item_compare2_not_keeping_zero),
+                        &info.item_compare_func_err);
+      if (info.item_compare_func_err) {
         EMSG(_("E702: Sort compare function failed"));
-      } else {
-        // Sort the array with item pointers.
-        qsort(ptrs, (size_t)len, sizeof (sortItem_T),
-              (info.item_compare_func == NULL
-               && info.item_compare_partial == NULL ?
-               item_compare_not_keeping_zero :
-               item_compare2_not_keeping_zero));
-
-        if (!info.item_compare_func_err) {
-          // Clear the list and append the items in the sorted order.
-          tv_list_clear(l);
-          for (i = 0; i < len; i++) {
-            tv_list_append(l, ptrs[i].item);
-          }
-        }
       }
     } else {
-      int (*item_compare_func_ptr)(const void *, const void *);
+      ListSorter item_compare_func_ptr;
 
       // f_uniq(): ptrs will be a stack of items to remove.
       info.item_compare_func_err = false;

src/nvim/eval/typval.c

@@ -3,6 +3,7 @@
 
 #include <stdio.h>
 #include <stddef.h>
+#include <stdlib.h>
 #include <string.h>
 #include <assert.h>
 #include <stdbool.h>
@@ -757,6 +758,47 @@ void tv_list_reverse(list_T *const l)
   l->lv_idx = l->lv_len - l->lv_idx - 1;
 }
 
+// FIXME Add unit tests for tv_list_item_sort().
+
+/// Sort list using libc qsort
+///
+/// @param[in,out]  l  List to sort, will be sorted in-place.
+/// @param  ptrs  Preallocated array of items to sort, must have at least
+///               tv_list_len(l) entries. Should not be initialized.
+/// @param[in]  item_compare_func  Function used to compare list items.
+/// @param  errp  Location where information about whether error occurred is
+///               saved by item_compare_func. If boolean there appears to be
+///               true list will not be modified. Must be initialized to false
+///               by the caller.
+void tv_list_item_sort(list_T *const l, ListSortItem *const ptrs,
+                       const ListSorter item_compare_func,
+                       bool *errp)
+  FUNC_ATTR_NONNULL_ARG(3, 4)
+{
+  const int len = tv_list_len(l);
+  if (len <= 1) {
+    return;
+  }
+  int i = 0;
+  TV_LIST_ITER(l, li, {
+    ptrs[i].item = li;
+    ptrs[i].idx = i;
+    i++;
+  });
+  // Sort the array with item pointers.
+  qsort(ptrs, (size_t)len, sizeof(ListSortItem), item_compare_func);
+  if (!(*errp)) {
+    // Clear the list and append the items in the sorted order.
+    l->lv_first    = NULL;
+    l->lv_last     = NULL;
+    l->lv_idx_item = NULL;
+    l->lv_len      = 0;
+    for (i = 0; i < len; i++) {
+      tv_list_append(l, ptrs[i].item);
+    }
+  }
+}
+
 //{{{2 Indexing/searching
 
 /// Locate item with a given index in a list and return it

src/nvim/eval/typval.h

@@ -296,6 +296,14 @@ typedef struct list_stack_S {
   struct list_stack_S *prev;
 } list_stack_T;
 
+/// Structure representing one list item, used for sort array.
+typedef struct {
+  listitem_T *item;  ///< Sorted list item.
+  int idx;  ///< Sorted list item index.
+} ListSortItem;
+
+typedef int (*ListSorter)(const void *, const void *);
+
 // In a hashtab item "hi_key" points to "di_key" in a dictitem.
 // This avoids adding a pointer to the hashtab item.
 
@@ -403,20 +411,6 @@ static inline list_T *tv_list_latest_copy(const list_T *const l)
   return l->lv_copylist;
 }
 
-/// Clear the list without freeing anything at all
-///
-/// For use in sort() which saves items to a separate array and readds them back
-/// after sorting via a number of tv_list_append() calls.
-///
-/// @param[out]  l  List to clear.
-static inline void tv_list_clear(list_T *const l)
-{
-  l->lv_first    = NULL;
-  l->lv_last     = NULL;
-  l->lv_idx_item = NULL;
-  l->lv_len      = 0;
-}
-
 static inline int tv_list_uidx(const list_T *const l, int n)
   REAL_FATTR_PURE REAL_FATTR_WARN_UNUSED_RESULT;