Added a null-terminating character to the character list when scanning a number.

[kfarnung]

This ensures that we won't overflow the heap while reading the scanned data.

[msftclas]

Hi @kfarnung, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Kyle Farnung). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

[kfarnung]

@mrkmarron Can you take a look?

[obastemur]

At the end of this method, charList is recreated with no null ending. I didn't follow the whole call chain but better we add null ending there too?

[mrkmarron]

In general we don't assume that the charList buffers are null terminated, the caller should insert a null terminator if needed, numbers are a special case. Specifically, adding a null terminator here will break the string parsing.

[obastemur]

Actually for DEBUG purposes, we might add a check if null_ending was ever added to charList and assert that on getBuffer call. This approach would give us an early warning on the consumer end.

@digitalinfinity was asking for a test implementation here, maybe this could be the approach to take ?

[kfarnung]

One thought I had was to add an assert to each location where we get the buffer in this code.

Something like:

TTDAssert(charList.Item(charList.Count() - 1) == '\0', "Char list must be null terminated");

[mrkmarron]

LGTM

[digitalinfinity]

LGTM too! 🏆

[kfarnung]

@mrkmarron I removed the other redundant null characters, can you take a second look?

[digitalinfinity]

@kfarnung @mrkmarron is it possible to come with a test that exposes this problem? If so, it would be great to add this to the unit tests

[dilijev]

Does this bug affect the release/1.4 branch? Is it likely to be a problem? Should we back-port this?

[kfarnung]

@dilijev The only currently existing issue is in the ScanNumber method. In that method the actual value is never, but only evaluated to determine whether we have encountered an error. Because the switch statement does the initial test and collects the first character, the only error case I can see that would come down this codepath would be a string containing only "-" or "+" as these would fail normally, but the garbage data after the end of the list could potentially contain a number and pass it anyway. Since this isn't expected and would indicate corrupted data, it's unlikely to have ever been hit.

This bug would exist in the 1.4 release, but it's a definite edge case that I believe has only been seen in static analysis tools.

/cc @digitalinfinity

[kfarnung]

@digitalinfinity @mrkmarron From a JS/TTD side I'm not coming up with anything. Are there any NativeUTs for this code? We could potentially exercise it that way with some memory manipulation of the incoming list.

[boingoing]

🚢

[mrkmarron]

I don't have any simple suggestions for adding a test case around this and this is a very low impact bug (so no need to port to 1.4). I am good with merging this as it.