Fixes reading past end of buffer for Google Earth terrain packets.

[markaubin]

Please review this fix to parsing Google Earth terrain packets.

[cesium-concierge]

Thank you so much for the pull request @markaubin! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

• ❌ Missing CONTRIBUTORS.md entry.
  • Please add yourself to the contributors file!
• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

[ebogo1]

@tfili Do you have any thoughts on these changes? Not sure if it's fine to have the option of stopping before reading all four parts of a tile.

[tfili]

@ebogo1 I need a little more info. It sounds like this code is protecting us from reading a incomplete tile. That is probably good to handle regardless. We definitely need a little more background and a test.

[tfili]

This prevents us from reading a part of a tile that doesn't exist, but if the 4th part is incomplete, the returned value will be out of range and it is used in the buffer.slice call below.

[markaubin]

This block returns a -1, which is tested against below and will break out of the while loop.

[tfili]

That will only account for the tile if there are less than 4 parts, but all parts are complete, which seems like it may be valid data. Is that correct?

My question was, do we need to account for partial quads, which I'm assuming would be an error? In that case, it would still fail.

For instance if a tile has a buffer of 98 bytes, but each part says it is 25 bytes. The function would read the 4th quad's size and return 100 and the slice would be out of bounds.

[markaubin]

I cannot say if partial quads are valid. From the existing code, it appears they are not. I think you are concerned with a malformed buffer which is a valid point and I will adjust the code to test immediately after the getUint32() call instead of before it. However in practice, that is not the problem we are experiencing. There seems to be extra bytes in the buffer after all valid tiles have been extracted.

[markaubin]

After some experimentation with partial tiles, and examining code that uses uses these terrain tiles, I'm quite certain that partial tiles are not supported.

The method interpolateHeight() assumes the terrain buffer has 4 parts without ever checking for buffer overruns.

cesium/Source/Core/GoogleEarthEnterpriseTerrainData.js

Line 466 in b333448

function interpolateHeight(terrainData, u, v, rectangle) {

[tfili]

Thanks @markaubin

It looks like a worthwhile change but we need an example of what triggered this issue and a unit test. I have the one other comment to be addressed as well.

[markaubin]

This processTerrain() method was written with the assumption that the packet contains n tiles, where each tile contains exactly 4 subparts. When this isn't true, which is frequently the case with the current live legacy Google Earth servers, the DataView.getUint32() call will throw an exception. This makes the entire packet fail parsing.

[tfili]

Wouldn't this be

if ((pos+sizeOfUint32+size) > totalSize) {

because size is just the size of the quad and will always be less than totalSize.

[markaubin]

Actually size is either a valid number of bytes for the tile subpart (quad) when the buffer still has tiles left to be parsed out, or it will be something else completely and this can easily be found because it is not a small number. The extra bytes in the terrain packet buffer are used for something else completely and not relevant to the CesiumJS viewer. Perhaps making the test as you suggest is slightly more correct and I will try that out.

[markaubin]

So after more digging in the legacy Google Earth client code, I have learned that the terrain packet will always have 5 tiles, and each tile will always have 4 sub-meshes. Any other combination means something went wrong and the parsing should be aborted. The remaining bytes after the 20 sub-meshes will be water surface meshes, which appear to be unused by CesiumJS so should be ignored. I will rework the parsing with this understanding.

[lilleyse]

@markaubin looks like there are two failing tests with the recent changes:

  Core/GoogleEarthEnterpriseTerrainProvider
    requestTileGeometry
      ✗ provides GoogleEarthEnterpriseTerrainData
	Expected 'requestTileGeometry' to be 'returning a tile.'.

      ✗ returns undefined if too many requests are already in progress
	Failed: promise rejected: TypeError: sharedRequest is undefined

[lilleyse]

From offline discussion, the test data that we have in Specs/Data/GoogleEarthEnterprise only has one tile with four sub-meshes and isn't conformant based on @markaubin's findings in #9519 (comment).

Looking back at the PR history and it seems like the original gee.terrain came from the public test server but was later replaced with a generated file. See https://github.com/CesiumGS/cesium/pull/5189/files#r110541815. I don't know what to do about attribution because the test server is down and I'm not sure where the data originally comes from (was it Google?).

Anyways here's the original file that works: gee.terrain.txt (remove the .txt extension).

@markaubin once you update that file, let us know and we can get this merged.

[lilleyse]

@markaubin one last thing, could you update CHANGES.md?

[lilleyse]

Thanks @markaubin!

[pjcozzi]

@markaubin congrats on your first CesiumJS pull request, thanks again for contributing!