@matheusbsbs / matheus-bagliano / commits / 19ceb486f8

merge-ort: avoid accidental API mis-use

Previously, callers of the merge-ort API could have passed an uninitialized value for struct merge_result *result. However, we want to check result to see if it has cached renames from a previous merge that we can reuse; such values would be found behind result->priv. However, if result->priv is uninitialized, attempting to access behind it will give a segfault. So, we need result->priv to be NULL (which will be the case if the caller does a memset(&result, 0)), or be written by a previous call to the merge-ort machinery. Documenting this requirement may help, but despite being the person who introduced this requirement, I still missed it once and it did not fail in a very clear way and led to a long debugging session. Add a _properly_initialized field to merge_result; that value will be 0 if the caller zero'ed the merge_result, it will be set to a very specific value by a previous run by the merge-ort machinery, and if it's uninitialized it will most likely either be 0 or some value that does not match the specific one we'd expect allowing us to throw a much more meaningful error. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed May 20, 2021 at 06:09 UTC 19ceb486f8dd25fb5782724c454edb2f06f1ed71

2 files changed +9

merge-ort.c

@@ -52,6 +52,8 @@ enum merge_side {

52 MERGE_SIDE2 = 2

53 };

55 +static unsigned RESULT_INITIALIZED = 0x1abe11ed; /* unlikely accidental value */

56 +

57 struct traversal_callback_data {

58 unsigned long mask;

59 unsigned long dirmask;

@@ -3768,6 +3770,10 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)

3770 assert(opt->obuf.len == 0);

3771

3772 assert(opt->priv == NULL);

3773 + if (result->_properly_initialized != 0 &&

3774 + result->_properly_initialized != RESULT_INITIALIZED)

3775 + BUG("struct merge_result passed to merge_incore_*recursive() must be zeroed or filled with values from a previous run");

3776 + assert(!!result->priv == !!result->_properly_initialized);

3777 if (result->priv) {

3778 opt->priv = result->priv;

3779 result->priv = NULL;

@@ -3927,6 +3933,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,

3933 result->clean &= strmap_empty(&opt->priv->conflicted);

3934 if (!opt->priv->call_depth) {

3935 result->priv = opt->priv;

3936 + result->_properly_initialized = RESULT_INITIALIZED;

3937 opt->priv = NULL;

3938 }

3939 }

merge-ort.h

@@ -29,6 +29,8 @@ struct merge_result {

29 * !clean) and to print "CONFLICT" messages. Not for external use.

30 */

31 void *priv;

32 + /* Also private */

33 + unsigned _properly_initialized;

34 };

36 /*