Re: [PATCH] for_each_string_list_item(): behave correctly for empty list

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 15, 2017, 18:43 UTC
Message-ID: <20170915184323.GU27425@aiede.mtv.corp.google.com>
In-Reply-To: <cb2d4d71c7c1db452b86c8076c153cabe7384e28.1505490776.git.mhagger@alum.mit.edu>

Hi,

Michael Haggerty wrote:

Show 11 quoted lines

> If you pass a newly-initialized or newly-cleared `string_list` to
> `for_each_string_list_item()`, then the latter does
>
>     for (
>             item = (list)->items; /* note, this is NULL */
>             item < (list)->items + (list)->nr; /* note: NULL + 0 */
>             ++item)
>
> Even though this probably works almost everywhere, it is undefined
> behavior, and it could plausibly cause highly-optimizing compilers to
> misbehave.

Wait, NULL + 0 is undefined behavior?

*checks the standard*  C99 section 6.5.6.8 says

	"If both the pointer operand and the result point to elements
	of the same array object, or one past the last element of the
	array object, the evaluation shall not produce an overflow;
	otherwise, the behavior is undefined."

C99 section 7.17.3 says

	"NULL

	which expands to an implementation-defined null pointer constant"

6.3.2.3.3 says

	"An integer constant expression with the value 0, or such an
	expression cast to type void *, is called a null pointer
	constant.  If a null pointer constant is converted to a
	pointer type, the resulting pointer, called a null pointer, is
	guaranteed to compare unequal to a pointer to any object or
	function."

NULL doesn't point to anything so it looks like adding 0 to a null pointer is indeed undefined. (As a piece of trivia, strictly speaking NULL + 0 would be undefined on some implementations and defined on others, since an implementation is permitted to #define NULL to 0.)

So Coverity is not just warning because it is not able to guarantee that list->nr is 0. Huh.

> It would be a pain to have to change the signature of this macro, and
> we'd prefer not to add overhead to each iteration of the loop. So
> instead, whenever `list->items` is NULL, initialize `item` to point at
> a dummy `string_list_item` created for the purpose.

What signature change do you mean? I don't understand what this paragraph is alluding to.

> This problem was noticed by Coverity.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

[...]

>  string-list.c | 2 ++
>  string-list.h | 7 +++++--
>  2 files changed, 7 insertions(+), 2 deletions(-)

Does the following alternate fix work? I think I prefer it because it doesn't require introducing a new global.

Thanks, Jonathan

diff --git i/string-list.h w/string-list.h
index 29bfb7ae45..dae33fbb89 100644
--- i/string-list.h
+++ w/string-list.h
@@ -33,7 +33,9 @@ typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t, void *cb_data);
 #define for_each_string_list_item(item,list) \
-	for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
+	for (item = (list)->items; \
+	     (list)->items && item < (list)->items + (list)->nr; \
+	     ++item)
 
 /*
  * Apply want to each item in list, retaining only the ones for which