Strings in dumped tombs sometimes start with null character

[edsrzf]

Occasionally, in our dumped tombs, we're seeing the first character of file names, scopes, or function names replaced with a null character. This corrupts our tombs and also produces invalid JSON since null characters are not allowed in strings. It looks something like this:

{"location": {"file": "\0opt/path/File.class.php", "start": 224, "end": 232}, "scope": "Scope", "function": "foo"}

(\0 represents the null character and is supposed to be / in this case, since it's the start of an absolute path.)

It occurs very rarely. I have a data set containing 1223752 tombs, and only 25 of them exhibit these null characters, and 18 of those 25 share the same string.

I believe this is happening due to a combination of things I'll try to explain below. I haven't been able to test for this, but I've walked through the code and it makes sense to me.

Off-by-One Buffer Overflow

When allocating space for strings in the string table, we do not take into account the null terminator.

We allocate ZSTR_LEN(string) bytes here: https://github.com/krakjoe/tombs/blob/40504a69bc965120ab0db078a2264642c326f820/zend_tombs_strings.c#L81

Then we write ZSTR_LEN(string) bytes here: https://github.com/krakjoe/tombs/blob/40504a69bc965120ab0db078a2264642c326f820/zend_tombs_strings.c#L102-L104

But then we write an additional null terminator here, which we did not allocate space for: https://github.com/krakjoe/tombs/blob/40504a69bc965120ab0db078a2264642c326f820/zend_tombs_strings.c#L106

Race Condition

Most of the time, the extra null terminator gets overwritten by the next string that's added to the table. And that doesn't end up mattering because we don't use the null terminator for anything. We already have a size.

Usually, the sequence of events for allocating and copying two strings looks like this:

1. Allocate space for s1: increment used by 2.
2. Copy s1\0 into string buffer.
3. Allocate space for s2: increment used by 2.
4. Copy s2\0 into string buffer.

The final content of the string buffer looks like: s1s2\0

But sometimes, the order of events is:

1. Allocate space for s1: increment used by 2.
2. Allocate space for s2: increment used by 2.
3. Copy s2\0 into string buffer.
4. Copy s1\0 into string buffer.

The final content of the string buffer looks like: s1\02\0

Whoops, we overwrote the first byte of the second string!

Possible Solutions

I can see two possible solutions:

1. Don't include the null terminator in the string buffer. We don't seem to need it, seeing as how it gets overwritten in 99.998% of cases anyway.
2. Add 1 to each string allocation to account for the space occupied by the null terminator.

I'd lean toward option 1, but unsure if there might be any downsides or risks to not having the null terminator.