Re: [PATCH 10/10] hashmap_for_each_entry(): work around MSVC's run-time check failure #3

Junio C Hamano <gitster@pobox.com> writes:

I looked at all the uses of OFFSETOF_VAR() and I think the one used for hashmap_for_each_entry() is the only instance that 'var' given to it can legitimately be uninitialized, if typeof() were available.

Here are the findings.

#define hashmap_put_entry(map, keyvar, member) \
	container_of_or_null_offset(hashmap_put(map, &(keyvar)->member), \
				OFFSETOF_VAR(keyvar, member))

The keyvar is a pointer to the entry being placed in the map; it must hold a valid one so the pointer-diff implementation of OFFSETOF_VAR() should work fine, or we are putting garbage in to the map.

#define hashmap_remove_entry(map, keyvar, member, keydata) \
	container_of_or_null_offset( \
			hashmap_remove(map, &(keyvar)->member, keydata), \
			OFFSETOF_VAR(keyvar, member))

The keyvar is used to match against an existing entry in the map to be removed---it must have a valid value.

#define hashmap_for_each_entry(map, iter, var, member) \
	for (var = hashmap_iter_first_entry_offset(map, iter, \
						OFFSETOF_VAR(var, member)); \
		var; \
		var = hashmap_iter_next_entry_offset(iter, \
						OFFSETOF_VAR(var, member)))

This, as you discovered, can be fed an uninitialized var and the first thing it does is to use OFFSETOF_VAR() on it in order to call hashmap_iter_first_entry_offset(). After that, i.e. when we called that function to start the loop, var is defined and we would be OK.

The trick I suggested is to initialize var to NULL before making the call to hashmap_iter_first_entry_offset(), i.e.

	for (var = NULL, \
	     var = hashmap_iter_first_entry_offset(map, iter, \
						OFFSETOF_VAR(var, member)); \

#define hashmap_get_entry(map, keyvar, member, keydata) \
	container_of_or_null_offset( \
				hashmap_get(map, &(keyvar)->member, keydata), \
				OFFSETOF_VAR(keyvar, member))

Must be OK for the same reason _put_entry() is OK.

#define hashmap_get_next_entry(map, var, member) \
	container_of_or_null_offset(hashmap_get_next(map, &(var)->member), \
				OFFSETOF_VAR(var, member))

This tries to go to the next-equal-pointer starting from var, so var must be valid already.

So, perhaps the attached may be a viable replacement that would be more futureproof with less maintenance cost, I suspect.

Thanks.

--- >8 ----- cut here ----- >8 ---
Subject: hashmap_for_each_entry(): workaround MSVC's runtime check failure #3

The OFFSETOF_VAR(var, member) macro is implemented in terms of offsetof(typeof(*var), member) with compilers that know typeof(), but its fallback implemenation compares &(var->member) and (var) and count the distance in bytes, i.e.

    ((uintptr_t)&(var)->member - (uintptr_t)(var))

MSVC's runtime check, when fed an uninitialized 'var', flags this as a use of an uninitialized variable (and that is legit---uninitialized contents of 'var' is subtracted) in a debug build.

After auditing all 6 uses of OFFSETOF_VAR(), 1 of them does feed a potentially uninitialized 'var' to the macro in the beginning of the for() loop:

    #define hashmap_for_each_entry(map, iter, var, member) \
            for (var = hashmap_iter_first_entry_offset(map, iter, \
                                                    OFFSETOF_VAR(var, member)); \
                    var; \
                    var = hashmap_iter_next_entry_offset(iter, \
                                                    OFFSETOF_VAR(var, member)))

We can work around this by making sure that var has _some_ value when OFFSETOF_VAR() is called. Strictly speaking, it invites undefined behaviour to use NULL here if we end up with pointer comparison, but MSVC runtime seems to be happy with it, and most other systems have typeof() and don't even need pointer comparison fallback code.

---
 hashmap.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)