git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[RFC PATCH 00/10] range-diff: fix segfault due to integer overflow

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 9, 2021, 19:19 UTC
Message-ID
<RFC-cover-00.10-00000000000-20211209T191653Z-avarab@gmail.com>

The difference between "master" and "git-for-windows/main" is large enough that comparing the two will segfault on my system. This is because the range-diff code does some expensive calculations and will overflow the "int" type.

Fixing this wasn't trivial, our st_*() macros only work for unsigned types, but as signed overflow is undefined in C detecting it is a lot more painful. The range-diff code needs to store -1 in various places (not inherently, but changing that looked a lot more painful).

Furthermore even if the range-diff.c and linear-assignment.c code were fixed we'd still segfault due to the used string-list.c code using an "int" type. Or rather, we tracked its "unsigned int" with an "int". If we used "unsigned" we won't segfault on master..git-for-windows/main, but we would soon enough on slightly larger divergent history.

This series changes the relevant APIs to use size_t where possible, but as noted we need to use a signed type for range-diff.c. That's now intmax_t.

We detect signed overflow first 8/10 with a GCC and Clang-specific __builtin*() function, and then in the subsequent commit portably by importing intprops.h from Gnulib.

Perhaps there's an easier way to do this, but this works for me, and using the portable intprops.h (see its source for all the hard-won lessons encdoded therein) seems like a good way forward. It *is* slightly slower than our current unsigned-only detection, but as discussed in 09/10 we should probably just switch to it anyway. This series does not make that switch.

We still have various st_*() calls in the codebase where we use signed types, presumably this is as broken as the not-really-working detection discussed in 02/10, but I didn't looki into those in any detail.

This is an RFC because there's some rather painful merge conflicts in-flight due to the changeover from "unsigned int nr" to "size_t nr" in the string-list.h API. There's also a CI failure on 32 bit linux due to a format in 03/10, but it's an easily fixable bug.

And more generally it's an RFC perhaps this direction is a good one for fixing that and any similar segfauls we have, and maybe it isn't. I'm not all that sure about it, but this seems to work, and certainly beats segfaulting...

Ævar Arnfjörð Bjarmason (10):
  string-list API: change "nr" and "alloc" to "size_t"
  range-diff.c: don't use st_mult() for signed "int"
  range-diff.c: use "size_t" to refer to "struct string_list"'s "nr"
  range-diff: zero out elements in "cost" first
  linear-assignment.c: split up compute_assignment() function
  linear-assignment.c: take "size_t", not "int" for *_count
  linear-assignment.c: convert a macro to a "static inline" function
  linear-assignment.c: detect signed add/mul on GCC and Clang
  linear-assignment.c: add and use intprops.h from Gnulib
  linear-assignment.c: use "intmax_t" instead of "int"
 builtin/receive-pack.c       |   6 +-
 builtin/shortlog.c           |   8 +-
 bundle.c                     |   4 +-
 commit-graph.c               |   4 +-
 compat/gnulib/.gitattributes |   1 +
 compat/gnulib/intprops.h     | 637 +++++++++++++++++++++++++++++++++++
 linear-assignment.c          | 149 +++++---
 linear-assignment.h          |   9 +-
 mailmap.c                    |   4 +-
 merge-ort.c                  |   2 +-
 range-diff.c                 |  32 +-
 string-list.h                |   2 +-
 t/helper/test-run-command.c  |   4 +-
 wt-status.c                  |   8 +-
 14 files changed, 781 insertions(+), 89 deletions(-)
 create mode 100644 compat/gnulib/.gitattributes
 create mode 100644 compat/gnulib/intprops.h
-- 
2.34.1.930.g0f9292b224d
Next: Ævar Arnfjörð Bjarmason
Message 1 of 44 in “range-diff: fix segfault due to integer overflow”
  1. 00/10 range-diff: fix segfault due to integer overflowÆvar Arnfjörð Bjarmason, Dec 9, 2021
  2. 01/10 string-list API: change "nr" and "alloc" to "size_t"Ævar Arnfjörð Bjarmason, Dec 9, 2021
  3. 02/10 range-diff.c: don't use st_mult() for signed "int"Ævar Arnfjörð Bjarmason, Dec 9, 2021
  4. Jeff KingDec 10, 2021
  5. Ævar Arnfjörð BjarmasonDec 10, 2021
  6. Jeff KingDec 10, 2021
  7. Ævar Arnfjörð BjarmasonDec 10, 2021
  8. Phillip WoodDec 10, 2021
  9. Jeff KingDec 14, 2021
  10. Johannes SchindelinDec 10, 2021
  11. Ævar Arnfjörð BjarmasonDec 10, 2021
  12. Johannes SchindelinDec 11, 2021
  13. Ævar Arnfjörð BjarmasonDec 12, 2021
  14. Jeff KingDec 14, 2021
  15. 03/10 range-diff.c: use "size_t" to refer to "struct string_list"'s "nr"Ævar Arnfjörð Bjarmason, Dec 9, 2021
  16. 05/10 linear-assignment.c: split up compute_assignment() functionÆvar Arnfjörð Bjarmason, Dec 9, 2021
  17. 04/10 range-diff: zero out elements in "cost" firstÆvar Arnfjörð Bjarmason, Dec 9, 2021
  18. 06/10 linear-assignment.c: take "size_t", not "int" for *_countÆvar Arnfjörð Bjarmason, Dec 9, 2021
  19. 07/10 linear-assignment.c: convert a macro to a "static inline" functionÆvar Arnfjörð Bjarmason, Dec 9, 2021
  20. 08/10 linear-assignment.c: detect signed add/mul on GCC and ClangÆvar Arnfjörð Bjarmason, Dec 9, 2021
  21. Jeff KingDec 10, 2021
  22. 09/10 linear-assignment.c: add and use intprops.h from GnulibÆvar Arnfjörð Bjarmason, Dec 9, 2021
  23. 10/10 linear-assignment.c: use "intmax_t" instead of "int"Ævar Arnfjörð Bjarmason, Dec 9, 2021
  24. Jeff KingDec 10, 2021
  25. 0/5 range-diff: fix segfault due to integer overflowÆvar Arnfjörð Bjarmason, Dec 10, 2021
  26. 1/5 range-diff: zero out elements in "cost" firstÆvar Arnfjörð Bjarmason, Dec 10, 2021
  27. Jeff KingDec 14, 2021
  28. 2/5 linear-assignment.c: split up compute_assignment() functionÆvar Arnfjörð Bjarmason, Dec 10, 2021
  29. Jeff KingDec 14, 2021
  30. 3/5 linear-assignment.c: take "size_t", not "int" for *_countÆvar Arnfjörð Bjarmason, Dec 10, 2021
  31. Jeff KingDec 14, 2021
  32. 4/5 range-diff.c: rename "n" to "column_count" in get_correspondences()Ævar Arnfjörð Bjarmason, Dec 10, 2021
  33. Jeff KingDec 14, 2021
  34. 5/5 range-diff: fix integer overflow & segfault on cost[i + n * j]Ævar Arnfjörð Bjarmason, Dec 10, 2021
  35. Jeff KingDec 14, 2021
  36. Johannes SchindelinDec 10, 2021
  37. Ævar Arnfjörð BjarmasonDec 10, 2021
  38. Philip OakleyDec 21, 2021
  39. Ævar Arnfjörð BjarmasonDec 21, 2021
  40. Johannes SchindelinDec 22, 2021
  41. Jeff KingDec 22, 2021
  42. Philip OakleyDec 24, 2021
  43. Ævar Arnfjörð BjarmasonDec 24, 2021
  44. Philip OakleyDec 24, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.