From 5bbb733a1b16b0c9d2303c1464bf66ee434450ac Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 10 Feb 2024 21:26:54 +0800 Subject: vim-patch:9.1.0089: qsort() comparison functions should be transitive Problem: qsort() comparison functions should be transitive Solution: Do not subtract values, but rather use explicit comparisons Improve qsort() comparison functions There has been a recent report on qsort() causing out-of-bounds read & write in glibc for non transitive comparison functions https://www.qualys.com/2024/01/30/qsort.txt Even so the bug is in glibc's implementation of the qsort() algorithm, it's bad style to just use substraction for the comparison functions, which may cause overflow issues and as hinted at in OpenBSD's manual page for qsort(): "It is almost always an error to use subtraction to compute the return value of the comparison function." So check the qsort() comparison functions and change them to be safe. closes: vim/vim#13980 https://github.com/vim/vim/commit/e06e43766500ecb4cd1031fa16cf9cbebdb222c1 Co-authored-by: Christian Brabandt --- src/nvim/ex_cmds.c | 10 ++++------ src/nvim/mbyte.c | 4 +++- src/nvim/search.c | 12 ++++++++++-- src/nvim/spellsuggest.c | 4 ++-- src/nvim/window.c | 12 ++++++++++-- 5 files changed, 29 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c index e265dc139a..70146a8602 100644 --- a/src/nvim/ex_cmds.c +++ b/src/nvim/ex_cmds.c @@ -406,18 +406,16 @@ static int sort_compare(const void *s1, const void *s2) // number. if (sort_nr) { if (l1.st_u.num.is_number != l2.st_u.num.is_number) { - result = l1.st_u.num.is_number - l2.st_u.num.is_number; + result = l1.st_u.num.is_number > l2.st_u.num.is_number ? 1 : -1; } else { result = l1.st_u.num.value == l2.st_u.num.value ? 0 - : l1.st_u.num.value > l2.st_u.num.value - ? 1 - : -1; + : l1.st_u.num.value > l2.st_u.num.value ? 1 : -1; } } else if (sort_flt) { result = l1.st_u.value_flt == l2.st_u.value_flt - ? 0 : l1.st_u.value_flt > l2.st_u.value_flt - ? 1 : -1; + ? 0 + : l1.st_u.value_flt > l2.st_u.value_flt ? 1 : -1; } else { // We need to copy one line into "sortbuf1", because there is no // guarantee that the first pointer becomes invalid when obtaining the diff --git a/src/nvim/mbyte.c b/src/nvim/mbyte.c index 8583b236c7..fd353d8a67 100644 --- a/src/nvim/mbyte.c +++ b/src/nvim/mbyte.c @@ -2792,8 +2792,10 @@ static int tv_nr_compare(const void *a1, const void *a2) { const listitem_T *const li1 = tv_list_first(*(const list_T **)a1); const listitem_T *const li2 = tv_list_first(*(const list_T **)a2); + const varnumber_T n1 = TV_LIST_ITEM_TV(li1)->vval.v_number; + const varnumber_T n2 = TV_LIST_ITEM_TV(li2)->vval.v_number; - return (int)(TV_LIST_ITEM_TV(li1)->vval.v_number - TV_LIST_ITEM_TV(li2)->vval.v_number); + return n1 == n2 ? 0 : n1 > n2 ? 1 : -1; } /// "setcellwidths()" function diff --git a/src/nvim/search.c b/src/nvim/search.c index 273a924876..f666b07c72 100644 --- a/src/nvim/search.c +++ b/src/nvim/search.c @@ -3436,7 +3436,11 @@ static int fuzzy_match_str_compare(const void *const s1, const void *const s2) const int idx1 = ((fuzmatch_str_T *)s1)->idx; const int idx2 = ((fuzmatch_str_T *)s2)->idx; - return v1 == v2 ? (idx1 - idx2) : v1 > v2 ? -1 : 1; + if (v1 == v2) { + return idx1 == idx2 ? 0 : idx1 > idx2 ? 1 : -1; + } else { + return v1 > v2 ? -1 : 1; + } } /// Sort fuzzy matches by score @@ -3465,7 +3469,11 @@ static int fuzzy_match_func_compare(const void *const s1, const void *const s2) if (*str1 == '<' && *str2 != '<') { return 1; } - return v1 == v2 ? (idx1 - idx2) : v1 > v2 ? -1 : 1; + if (v1 == v2) { + return idx1 == idx2 ? 0 : idx1 > idx2 ? 1 : -1; + } else { + return v1 > v2 ? -1 : 1; + } } /// Sort fuzzy matches of function names by score. diff --git a/src/nvim/spellsuggest.c b/src/nvim/spellsuggest.c index 887ad3a62a..5c0e295f88 100644 --- a/src/nvim/spellsuggest.c +++ b/src/nvim/spellsuggest.c @@ -3221,10 +3221,10 @@ static int sug_compare(const void *s1, const void *s2) { suggest_T *p1 = (suggest_T *)s1; suggest_T *p2 = (suggest_T *)s2; - int n = p1->st_score - p2->st_score; + int n = p1->st_score == p2->st_score ? 0 : p1->st_score > p2->st_score ? 1 : -1; if (n == 0) { - n = p1->st_altscore - p2->st_altscore; + n = p1->st_altscore == p2->st_altscore ? 0 : p1->st_altscore > p2->st_altscore ? 1 : -1; if (n == 0) { n = STRICMP(p1->st_word, p2->st_word); } diff --git a/src/nvim/window.c b/src/nvim/window.c index a188d75000..15bd1212ad 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -7359,9 +7359,17 @@ static bool frame_check_width(const frame_T *topfrp, int width) } /// Simple int comparison function for use with qsort() -static int int_cmp(const void *a, const void *b) +static int int_cmp(const void *pa, const void *pb) { - return *(const int *)a - *(const int *)b; + const int a = *(const int *)pa; + const int b = *(const int *)pb; + if (a > b) { + return 1; + } + if (a < b) { + return -1; + } + return 0; } /// Handle setting 'colorcolumn' or 'textwidth' in window "wp". -- cgit From f3982ad3f3bc866c9369edfbd4125aaa092215a9 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 10 Feb 2024 21:31:44 +0800 Subject: vim-patch:9.1.0093: Still a qsort() comparison function that returns result of subtraction Problem: Still a qsort() comparison function fuzzy_match_item_compare() that returns result of subtraction (after 9.1.0089). Solution: Use an explicit comparison instead of subtraction. (zeertzjq) closes: vim/vim#14004 https://github.com/vim/vim/commit/77078276bfe695070441a1bbdc02949d31de8922 --- src/nvim/search.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/nvim/search.c b/src/nvim/search.c index f666b07c72..48e41c290d 100644 --- a/src/nvim/search.c +++ b/src/nvim/search.c @@ -3197,7 +3197,11 @@ static int fuzzy_match_item_compare(const void *const s1, const void *const s2) const int idx1 = ((const fuzzyItem_T *)s1)->idx; const int idx2 = ((const fuzzyItem_T *)s2)->idx; - return v1 == v2 ? (idx1 - idx2) : v1 > v2 ? -1 : 1; + if (v1 == v2) { + return idx1 == idx2 ? 0 : idx1 > idx2 ? 1 : -1; + } else { + return v1 > v2 ? -1 : 1; + } } /// Fuzzy search the string "str" in a list of "items" and return the matching -- cgit From 00e785b17fde8c476031e3c24ea77bed45b88a89 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Sat, 10 Feb 2024 21:38:48 +0800 Subject: refactor: don't use subtraction in qsort() comparison functions --- src/nvim/channel.c | 7 ++++--- src/nvim/decoration.c | 16 +++++++++++++--- src/nvim/normal.c | 2 +- src/nvim/shada.c | 4 +--- src/nvim/sign.c | 20 ++++++++++++++------ src/nvim/winfloat.c | 4 +++- 6 files changed, 36 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/nvim/channel.c b/src/nvim/channel.c index 05148801f4..1dc1208696 100644 --- a/src/nvim/channel.c +++ b/src/nvim/channel.c @@ -962,10 +962,11 @@ Dictionary channel_info(uint64_t id) } /// Simple int64_t comparison function for use with qsort() -static int int64_t_cmp(const void *a, const void *b) +static int int64_t_cmp(const void *pa, const void *pb) { - int64_t diff = *(int64_t *)a - *(int64_t *)b; - return (diff < 0) ? -1 : (diff > 0); + const int64_t a = *(const int64_t *)pa; + const int64_t b = *(const int64_t *)pb; + return a == b ? 0 : a > b ? 1 : -1; } Array channel_all_info(void) diff --git a/src/nvim/decoration.c b/src/nvim/decoration.c index 9742a2020a..9767c44897 100644 --- a/src/nvim/decoration.c +++ b/src/nvim/decoration.c @@ -673,10 +673,20 @@ int sign_item_cmp(const void *p1, const void *p2) { const SignItem *s1 = (SignItem *)p1; const SignItem *s2 = (SignItem *)p2; - int n = s2->sh->priority - s1->sh->priority; - return n ? n : (n = (int)(s2->id - s1->id)) - ? n : (s2->sh->sign_add_id - s1->sh->sign_add_id); + if (s1->sh->priority != s2->sh->priority) { + return s1->sh->priority < s2->sh->priority ? 1 : -1; + } + + if (s1->id != s2->id) { + return s1->id < s2->id ? 1 : -1; + } + + if (s1->sh->sign_add_id != s2->sh->sign_add_id) { + return s1->sh->sign_add_id > s2->sh->sign_add_id ? 1 : -1; + } + + return 0; } static const uint32_t sign_filter[4] = {[kMTMetaSignText] = kMTFilterSelect, diff --git a/src/nvim/normal.c b/src/nvim/normal.c index 8b6ef62873..d69e43e6b3 100644 --- a/src/nvim/normal.c +++ b/src/nvim/normal.c @@ -375,7 +375,7 @@ static int nv_compare(const void *s1, const void *s2) if (c2 < 0) { c2 = -c2; } - return c1 - c2; + return c1 == c2 ? 0 : c1 > c2 ? 1 : -1; } /// Initialize the nv_cmd_idx[] table. diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 61c43e271b..2c8685adc7 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -1875,9 +1875,7 @@ static int compare_file_marks(const void *a, const void *b) const FileMarks *const *const b_fms = b; return ((*a_fms)->greatest_timestamp == (*b_fms)->greatest_timestamp ? 0 - : ((*a_fms)->greatest_timestamp > (*b_fms)->greatest_timestamp - ? -1 - : 1)); + : ((*a_fms)->greatest_timestamp > (*b_fms)->greatest_timestamp ? -1 : 1)); } /// Parse msgpack object that has given length diff --git a/src/nvim/sign.c b/src/nvim/sign.c index 9caacd8115..bd7979153e 100644 --- a/src/nvim/sign.c +++ b/src/nvim/sign.c @@ -172,20 +172,28 @@ int sign_cmp(const void *p1, const void *p2) { const MTKey *s1 = (MTKey *)p1; const MTKey *s2 = (MTKey *)p2; - int n = s1->pos.row - s2->pos.row; - if (n) { - return n; + if (s1->pos.row != s2->pos.row) { + return s1->pos.row > s2->pos.row ? 1 : -1; } DecorSignHighlight *sh1 = decor_find_sign(mt_decor(*s1)); DecorSignHighlight *sh2 = decor_find_sign(mt_decor(*s2)); assert(sh1 && sh2); - n = sh2->priority - sh1->priority; + if (sh1->priority != sh2->priority) { + return sh1->priority < sh2->priority ? 1 : -1; + } + + if (s1->id != s2->id) { + return s1->id < s2->id ? 1 : -1; + } + + if (sh1->sign_add_id != sh2->sign_add_id) { + return sh1->sign_add_id < sh2->sign_add_id ? 1 : -1; + } - return n ? n : (n = (int)(s2->id - s1->id)) - ? n : (sh2->sign_add_id - sh1->sign_add_id); + return 0; } /// Delete the specified sign(s) diff --git a/src/nvim/winfloat.c b/src/nvim/winfloat.c index f22c0f3cfa..1c9eb4ec5c 100644 --- a/src/nvim/winfloat.c +++ b/src/nvim/winfloat.c @@ -233,7 +233,9 @@ void win_config_float(win_T *wp, WinConfig fconfig) static int float_zindex_cmp(const void *a, const void *b) { - return (*(win_T **)b)->w_float_config.zindex - (*(win_T **)a)->w_float_config.zindex; + int za = (*(win_T **)a)->w_float_config.zindex; + int zb = (*(win_T **)b)->w_float_config.zindex; + return za == zb ? 0 : za < zb ? 1 : -1; } void win_float_remove(bool bang, int count) -- cgit