Bug: SegFault caused by invalid string

[oobayly]

I came across this when adding a large number of attributes to a nodes. In my case I was adding all the OSM tags with a railway: prefix (I'm not planning on doing always doing this, but it's useful for development). This was fine for a small region (Köln Rebenz), but would end with SIGSEGV when using the parent area (Nordrehin Westfalen).

My first thought was that it's an OOM error, but this turned out not to be the case:

• First, I used the --store flag, but this made no difference.
• The total memory being used was only 3.5GB. Docker has up to 32GB on my machine with a total of 64GB physical memory.
• Increasing the number of nodes greatly (by processing the entirety of Germany), but with only subsets of the railways: tags did work, with a similar 3.5GB usage.
• At one stage (I can't remember how), I managed to get a readable error that suggested a string issue.

So, it seems to be an issue with attributes, I:

• Rebuilt tilemaker with debug and didn't strip the symbols
• Enabled coredumps in docker and installed gdb
• Made sure to run with only a single thread
• Managed obtain a backtrace for the offending error - attached tilemaker-backtrace.txt
• In my case there are 301 items in the AttributeStore->keyStore. I appreciate that this many not be great practice, but nor should this be causing a SegFault.

In short, it was error reading variable: Cannot access memory at address 0x1a149fe, and was been thrown in OutputObject::writeAttributes:49

Attaching a debugger, I found that address of the string const std::string& key = attributeStore.keyStore.getKeyUnsafe(it->keyIndex); was invalid, and attempting to read it is causing an access violation. More specifically, the iterator it had an invalid (negative) keyIndex value. What's interesting is that the other properties for the AttributePair is valid, valueType and the string in stringValue.

I'll try setting up a simple (but completely unrealistic) set up so it can be reproduced on demand.

As an aside, I'm going to create another 2 issues with associated PR that helped me with getting live debugging to work via docker.

• 751

• 752

[cldellow]

D'oh, sorry that you're hitting this. I can offer some context.

In https://github.com/systemed/tilemaker/pull/583/, I changed AttributePair to identify the key by a short rather than a std::string. This had the effect of limiting us to 64K unique keys, which is probably plenty.

In https://github.com/systemed/tilemaker/pull/618, I further changed it to use at most 9 bits: https://github.com/systemed/tilemaker/blob/eab08d189ad97ddf5db7d915bcabe42ad3dab6af/include/attribute_store.h#L49

That limits us to at most 512 unique keys.

Except... this has the same bug as https://github.com/systemed/tilemaker/pull/672 did: it's using a signed type. Instead of short : 9, it should be unsigned short : 9. As it stands, the usable range is -256..255, not 0..511, so you can actually only squeeze in 256 keys before hitting issues.

So probably there are at least two things that ought to be done:

1. Use unsigned short : 9 so we get the full range.
2. Consider making AttributeKeyStore assert if it would return an index that would be out-of-bounds (so that future people who hit this issue get a useful error, not a segfault)

And then possibly:

3. Permit more than 512 keys to be used, at the expense of increasing the size of AttributePair. Maybe in your scenario that's not needed?

[oobayly]

@cldellow Nice. My first thought was that it was an overflow, given the unexpected number, but the bounds (-245) just didn't make sense to me. Too big for 8 bit, too small for 16 bit. The daft part is that when looking at the definition short keyIndex : 9; I did wonder about the syntax (I'm not a C/C++ guy, should have googled it though).

Just to verify this...

1. Because, I know the ID of the element, and that the tag value is branch, which means the tag key will be usage and the keyIndex = -245
2. The keyIndex requested was 267 (based on the two's complement
3. Boom, the tag at index 267 is "usage"
4. Also works for the next attribute which was maxspeed with a index of -244

Feeling pretty daft that I didn't work that one out 🤦

So, with regards to this:

1. I've just rebuilt tilemaker changing keyIndex to an unsigned short, and it looks up the indexes without a worry, even processing the stupid amount of attributes for the entirety of Germany
2. That would be ideal, probably better if somebody other than I does that...
3. "512 attributes ought to be enough for anyone"... Jokes aside, I'll leave that one up to people better at this me. 512 is plenty for my use, I'm not planning on having as many as want to abstract many of the attributes I'm chucking on to the tiles.

[cldellow]

Great, thanks for verifying the issue! I opened #760 to apply that fix + improve the failure mode. I've left the limit at 512 - if/when someone complains that it's not enough, we can revisit improving it.