memory corruption in Vanilla

[jbu]

First post. I spent 16 secs on reading the FAQ, so i am pretty sure I am not violating any forum rules

I am an oldie; played Angband almost when it started and for quite a few years thereafter. I wanted to see what happened over the years, so I just checked the code out. I managed to make it compile under VS2005, and made a few tweaks.

One of the tweaks was to show object values in the examine command. I started noticing that sometimes the value was completely off. Yes sir, I would like a few million for that arrow! I could not find the error and decided to use PageHeap (gflags) fom WinDbg. It interrupted when I tried to shoot when there was no target (the TAB).

The offending code is in target_set_closest()


/* Get ready to do targetting */
target_set_interactive_prepare(mode);

/* Find the first monster in the queue */
y = temp_y[0];
x = temp_x[0];
m_idx = cave_m_idx[y][x];

/* Target the monster, if possible */
if ((m_idx <= 0) || !target_able(m_idx))
{
msg_print("No Available Target.");
return FALSE;
}

target_set_interactive_prepare runs through all the 'interesting' locations and
sticks the sorted locations in the global(!?) arrays temp_x and temp_y.

If there are no targets, then the temp_xy[0] values will be undefined and may or may not be out of bounds for cave_m_idx. The check afterwards will probably fail and the warning show and the function returns.

This error is probably not the cause of my original error, since memory is just read. The hunt will go on for a few days until I get bored or I find the error. I can publish other related problems, if found, here if that is the prupose of this forum?

Cheers

[Magnate]

Welcome, and thank you - that's helpful to know. Yes it is the purpose of this forum, and yes please do let us know if you find the cause of the pricing weirdness. We're aware of it (http://trac.rephial.org/ticket/1160) but have not yet been able to track it down or reproduce it.

[d_m]

Thanks for catching this! I just checked in a fix as r2022.

I think the functions which use those values did sanity checks so nothing bad was happening, but using uninitialized memory is obviously bad.

Can you confirm that this fixes it for you? It seems to for me.

[Jungle_Boy]

I like the idea of showing the object value when examining. can we add that as a feature request?

[jbu]

duplicate, sorry

[jbu]

I think I got it. The value calculation depends of the slay_power() function in obj-power.c. This function caches the slay_value for a given flag-set in an array. This array is initialised in eval_e_slays() in init1.c. The initialisation use the flag-combos in ego items for the possible flags.

	for (i = 0; i < z_info->e_max; i++)
	{
		if (!of_is_empty(dupcheck[i]))
		{
			of_copy(slay_cache[count].flags, dupcheck[i]);
			slay_cache[count].value = 0;
			count++;
			/*msg_print("Cached a slay combination");*/
		}
	}

But the slay_power() calculation is done for all kinds of items, which are not guaranteed to conform to the specific ego_item flag combinations. Most prominent are the artifact gear. The usage of the cache is a bit strange. If a flag_combination is not known (and normal items with 0 flags are unknown), the first element after the known flag-combos is used.

	for (i = 0; !of_is_empty(slay_cache[i].flags); i++)
	{
		if (of_is_equal(s_index, slay_cache[i].flags))
			break;
	}

	sv = slay_cache[i].value;
 
	if (sv)
	{
		LOG_PRINT("Slay cache hit\n");
		return sv;
	}

/*sv is calculated and the cache entry is set*/

	/* Add to the cache */
	for (i = 0; !of_is_empty(slay_cache[i].flags); i++)
	{
		if (of_is_equal(s_index, slay_cache[i].flags))
			break;
	}

	slay_cache[i].value = sv;

If the first usage of slay_power is used with something with unknown flag-combo the last+1 entry in the cache will be populated with the slay power which may be high. But the flag combination is not recorded and when the function is used next time for a normal item, it will have that power, and thus be over-priced.

I was able to test this by getting the price of Careth Asdriag first and then go into the armor shop. A robe[2,0] cost 12880. The problem is that it is not easy to get to this situation in Vanilla because the only(?) place you see prices is in shops, and it must be the first item in the shop which is 'unknown' to the cache. But it looks like in wizard mode there is a command that does the calculation. I was able to see it because I extended the examine command with the value information as described in my first post.

I will suggest to drop the cache completely. The calculations are not that cumbersome and are only invoked when entering a shop, or, in my case, when examining an object.

Otherwise, I will suggest a more pragmatic approach, where the cache is allocated and zeroed in the init, but the cache is established when unknown flag-combos enter the slay_power() function:

	static count = 0;

 	/* Look in the cache to see if we know this one yet */
	for (i = 0; i < count; i++) //flags is a zero pointer when initialised
	{
		if (of_is_equal(s_index, slay_cache[i].flags)){
			found = TRUE;
			break;
		}
	}
	if (found)
	{
		LOG_PRINT("Slay cache hit\n");
		return slay_cache[i].value;
	}
/*  slay value is calculated and the cache entry is set */
	/* Add to the cache */

	slay_cache[count].value = sv;
	of_copy(slay_cache[count].flags, s_index);
	count++;

It has been years since I wrote C, so please disregard any errors or bad style.

I think it would be even better to make the slay_cache array as a static inside the function, and memset first time it is entered. I dislike the way angband have arrays with global visibility.

Cheers

[jbu]

Damn, the indentation of that post sucks. Oh, well.

[Rizwan]

Damn, the indentation of that post sucks. Oh, well.

You could use the code tags, it might help.

[jbu]

Thanks, did not know there was one. Done

[Magnate]

Thank you for the diagnosis - this is really helpful. The idea of the slay cache is to cache only those slay combinations found in ego_item.txt, as they are likely to occur much more commonly than those on artifacts. Unknown slay combinations, such as those on artifacts, are not supposed to be cached at all - that's where the problem is. The adding-to-cache line ended up outside the if block when it should have been inside (along with the log statement). This is fixed in r2027 - many thanks for catching it.

@Jungle_Boy: I'm afraid Takkaria has explicitly rejected the request to show object value in the inspection screen, because he doesn't want to encourage selling.