different segfault with lastest master

[Linus Torvalds]

On Mon, Jul 13, 2015 at 1:39 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> The idea is that the taxomony should always be consistent. NULL pointer
> and nothing there, or .nr in sync with the number of valid pointers.

Yes, yes. But if that initialization never happens, then you may have
several *old* taxonomy values in there. "nr" is in sync with the
number of valid pointers, that's not the problem.

The problem is that "nr" is not necessarily 0 or 1, because we may
have *previous* taxonomy data, and we keep potentially growing it.

> The fact that this part is conditional should not matter.

Dirk. Please. Read my emails. Or read the code.

That conditional matters, exactly because if we don't hit it, then we
won't be re-initializing the number of taxonomy entries at all.

Which in turn I think can be a pioblem because the code then later on does this:

  ds->taxonomy.category[ds->taxonomy.nr].category = TC_OCEAN;
  ds->taxonomy.category[ds->taxonomy.nr].origin = taxonomy::GEOCODED;
  ds->taxonomy.category[ds->taxonomy.nr].value =
copy_string(qPrintable(oceanName["name"].toString()));
  ds->taxonomy.nr++;

and that code at no point checks that "taxonomy.nr" was in the proper
range. We migth have filled the array already.

And dammit, I sent you a valgrind report of those three writes WRITING
TO PAST THE END OF THE ALLOCATION.

So IT DOES NOT MATTER if "nr is in sync with the number of valid
pointers". If "nr" is too big, the code is buggy and overwrites the
end. And if that re-initialization of "nr" never happened, we migth
not have reset it to some small value.

And according to valgrid, we *know* it overflows.

Really. Go back a few emails and look at my attachment with the
valgrind report. Valgrind says:

==9911== Invalid write of size 4
==9911==    at 0x6A92CC: ReverseGeoLookupThread::run() (divesitehelpers.cpp:155)
...
==9911==  Address 0x27cd71f8 is 0 bytes after a block of size 168 alloc'd
==9911==    at 0x4C2A987: calloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9911==    by 0x6AB2D7: alloc_taxonomy (taxonomy.c:28)

That like 155 is that

  ds->taxonomy.category[ds->taxonomy.nr].category = TC_OCEAN;

(and the two other reports are the next two lines)

                 Linus