Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Fix too-short {prefix} evaluation #13429

Merged
merged 1 commit into from
Nov 21, 2018
Merged

Fix too-short {prefix} evaluation #13429

merged 1 commit into from
Nov 21, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Nov 21, 2018

Fixed an issue where the {prefix} token in tile URL templates was evaluated incorrectly when requesting a source’s tiles. The prefix ended up being one character long, even though it should be two characters long (compare with GL JS):

<td><code>{prefix}</code></td>
<td>Two hexadecimal digits chosen such that each visible tile has a
different prefix. The prefix is typically used for domain sharding.</td>

This appears to have been an attempt to use the std::string fill constructor, but it ended up creating a one-character-long string consisting of and attempting to overwrite the null terminator, which resulted in a one-character-long prefix and a tile that couldn’t be found.

/cc @kkaefer @tobrun

@1ec5 1ec5 added bug Core The cross-platform C++ core, aka mbgl labels Nov 21, 2018
@1ec5 1ec5 added this to the release-iowaska milestone Nov 21, 2018
@1ec5 1ec5 self-assigned this Nov 21, 2018
@1ec5 1ec5 requested a review from kkaefer November 21, 2018 01:10
@@ -123,7 +123,7 @@ Resource Resource::tile(const std::string& urlTemplate,
} else if (token == "bbox-epsg-3857") {
return getTileBBox(x, y, z);
} else if (token == "prefix") {
std::string prefix{ 2 };
std::string prefix{ "00" };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, good catch. It looks like this is a pretty bad trap in the std::string constructor. Instead of choosing the string(size_t, char) constructor that I intended, it implicitly converted 2 to a std::initializer_list<char> and chose string(std::initializer_list<char>). This was only possibly because I was using list initialization with braces, which chooses the initializer_list constructor in this case. This wouldn't have happened with std::string prefix(2) (compiler error). Thanks for catching this! I assume our memory sanitizer didn't catch this because we weren't technically writing out of bounds; just overwriting the null terminator in the string.

This appears to have been an attempt to use the std::string fill constructor, but it ended up creating a one-character-long string and attempting to overwrite the null terminator.
@1ec5 1ec5 force-pushed the 1ec5-resource-prefix branch from 38dfe2b to e7def65 Compare November 21, 2018 16:45
@1ec5 1ec5 merged commit 7d5be98 into master Nov 21, 2018
@1ec5 1ec5 deleted the 1ec5-resource-prefix branch November 21, 2018 17:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants