Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix null-pointer dereference MapRenderer Android #2631

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

louwers
Copy link
Collaborator

@louwers louwers commented Jul 17, 2024

In the Android-specific MapRenderer class a std::unique_ptr<AndroidRendererBackend> is dereferenced that may be a nullptr. This can of course lead to a crash, which was reported in #2579

I am surprised this has not resulted in more crashes being reported, because I think the null-pointer dereference happens every time onSurfaceCreated is (initially) called, and a method is even called on the dereferenced null-pointer...

We should probably release 11.1.0 once this is released merged.

@TimSylvester
Copy link
Collaborator

Interesting, it looks like that means a render can occur before onSurfaceCreated is called or after a resetRenderer.

@louwers
Copy link
Collaborator Author

louwers commented Jul 18, 2024

Interesting, it looks like that means a render can occur before onSurfaceCreated is called or after a resetRenderer.

I think onSurfaceCreated dereferences backend before it is initialized in that same method.

@kodebach
Copy link
Contributor

I think onSurfaceCreated dereferences backend before it is initialized in that same method.

I rather suspect it's the other end of the lifecycle, i.e. render is called after Android has destroyed the surface. (see also #2579 (comment))

I don't know the maplibre code, so no idea if Android destroying the surface would cause the backend reference to become invalid. But based on our crash data that seems most likely to me.

@louwers
Copy link
Collaborator Author

louwers commented Jul 18, 2024

Right. When render is called after resetRenderer, the backend is again a nullptr.

@louwers
Copy link
Collaborator Author

louwers commented Jul 18, 2024

When the app goes to the background and onSurfaceDestroyed is called, the backend and renderer are reset. onSurfaceChanged calls onSurfaceCreated when no renderer exists, but onSurfaceCreated needs a valid backend to exist. I updated the reset() method to create a new backend to make sure there is always a valid one.

@louwers louwers merged commit f188a65 into maplibre:main Jul 18, 2024
20 checks passed
@louwers louwers deleted the fix-ub-maprenderer-android branch July 18, 2024 14:39
@louwers louwers mentioned this pull request Jul 18, 2024
TimSylvester added a commit to WetDogWeather/maplibre-gl-native that referenced this pull request Oct 15, 2024
@elican-doenyas
Copy link

@louwers I experienced the same issue when I upgraded to use the latest map libre android sdk (v11.6.1). Looking into the issue, there seems to be a nullptr dereference on MapRenderer:render method. I believe this happens when the style.json does not include a renderer property in its metadata. I was able to resolve the issue by adding;

"maputnik:renderer": "mbgljs"

value to the metadata section of the style.json I am using.

Can you please verify this and possibly introduce a fix so it does not crash in the absence of this property? We were not aware that it was required as the metadata is listed as 'optional' in the style specs here: https://maplibre.org/maplibre-style-spec/root/#metadata

Note 1: Previously, our company was using the v10.2.0 maplibre android sdk, and it was working fine without this property in the style.

Note 2: The relevant crash log is attached.
maplibre-nullptr.txt

@louwers
Copy link
Collaborator Author

louwers commented Nov 20, 2024

@elican-doenyas That is a different issue, please open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants