From b0e2e2ce852352d100e541a937ee3b543415e330 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Fri, 6 Jun 2025 23:38:19 +0200 Subject: [PATCH] Fix segfault this-capture GeoJsonSource which may be deleted (#3536) Co-authored-by: Tim Sylvester Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- include/mbgl/style/source.hpp | 12 +++++-- .../style/sources/custom_geometry_source.cpp | 10 +++--- .../src/cpp/style/sources/geojson_source.cpp | 28 +++++++++------ .../src/cpp/style/sources/geojson_source.hpp | 4 +-- .../src/cpp/style/sources/image_source.cpp | 8 ++--- .../cpp/style/sources/raster_dem_source.cpp | 2 +- .../src/cpp/style/sources/raster_source.cpp | 2 +- .../src/cpp/style/sources/source.cpp | 34 +++++++++---------- .../src/cpp/style/sources/source.hpp | 14 ++++++-- .../src/cpp/style/sources/vector_source.cpp | 4 +-- .../testapp/style/GeoJsonSourceTests.java | 24 +++++++++++++ 11 files changed, 96 insertions(+), 46 deletions(-) diff --git a/include/mbgl/style/source.hpp b/include/mbgl/style/source.hpp index db3b7063b..456cff0ab 100644 --- a/include/mbgl/style/source.hpp +++ b/include/mbgl/style/source.hpp @@ -55,13 +55,21 @@ public: /// Dynamically cast this source to the given subtype. template + requires(std::is_base_of_v) T* as() { - return is() ? reinterpret_cast(this) : nullptr; + if constexpr (std::is_same_v) { + return this; + } + return is() ? static_cast(this) : nullptr; } template + requires(std::is_base_of_v) const T* as() const { - return is() ? reinterpret_cast(this) : nullptr; + if constexpr (std::is_same_v) { + return this; + } + return is() ? static_cast(this) : nullptr; } SourceType getType() const; diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/custom_geometry_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/custom_geometry_source.cpp index 79b3116ef..25cdd9798 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/custom_geometry_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/custom_geometry_source.cpp @@ -141,18 +141,18 @@ void CustomGeometrySource::setTileData( // Update the core source if not cancelled if (!isCancelled(z, x, y)) { - source.as()->CustomGeometrySource::setTileData(CanonicalTileID(z, x, y), - GeoJSON(geometry)); + getSource().CustomGeometrySource::setTileData(CanonicalTileID(z, x, y), + GeoJSON(geometry)); } } void CustomGeometrySource::invalidateTile(jni::JNIEnv&, jni::jint z, jni::jint x, jni::jint y) { - source.as()->CustomGeometrySource::invalidateTile(CanonicalTileID(z, x, y)); + getSource().CustomGeometrySource::invalidateTile(CanonicalTileID(z, x, y)); } void CustomGeometrySource::invalidateBounds(jni::JNIEnv& env, const jni::Object& jBounds) { auto bounds = LatLngBounds::getLatLngBounds(env, jBounds); - source.as()->CustomGeometrySource::invalidateRegion(bounds); + getSource().CustomGeometrySource::invalidateRegion(bounds); } jni::Local>> CustomGeometrySource::querySourceFeatures( @@ -162,7 +162,7 @@ jni::Local>> CustomGeometrySource::quer std::vector features; if (rendererFrontend) { - features = rendererFrontend->querySourceFeatures(source.getID(), {{}, toFilter(env, jfilter)}); + features = rendererFrontend->querySourceFeatures(getSource().getID(), {{}, toFilter(env, jfilter)}); } return Feature::convert(env, features); } diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp index bc37a5f5c..cb51a1326 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp @@ -50,12 +50,12 @@ GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, const jni::String& sourceId, cons std::make_unique(jni::Make(env, sourceId), convertGeoJSONOptions(env, options))), converter(std::make_unique>(Scheduler::GetBackground(), - source.as()->impl().getOptions())) {} + getSource().impl().getOptions())) {} GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, AndroidRendererFrontend* frontend) : Source(env, coreSource, createJavaPeer(env), frontend), converter(std::make_unique>(Scheduler::GetBackground(), - source.as()->impl().getOptions())) {} + getSource().impl().getOptions())) {} GeoJSONSource::~GeoJSONSource() = default; @@ -83,11 +83,11 @@ void GeoJSONSource::setGeometry(jni::JNIEnv& env, const jni::Object()->setURL(jni::Make(env, url)); + getSource().setURL(jni::Make(env, url)); } jni::Local GeoJSONSource::getURL(jni::JNIEnv& env) { - std::optional url = source.as()->getURL(); + std::optional url = getSource().getURL(); return url ? jni::Make(env, *url) : jni::Local(); } @@ -98,7 +98,7 @@ jni::Local>> GeoJSONSource::querySource std::vector features; if (rendererFrontend) { - features = rendererFrontend->querySourceFeatures(source.getID(), {{}, toFilter(env, jfilter)}); + features = rendererFrontend->querySourceFeatures(getSource().getID(), {{}, toFilter(env, jfilter)}); } return Feature::convert(env, features); } @@ -112,7 +112,7 @@ jni::Local>> GeoJSONSource::getClusterC mbgl::Feature _feature = Feature::convert(env, feature); _feature.properties["cluster_id"] = static_cast(_feature.properties["cluster_id"].get()); const auto featureExtension = rendererFrontend->queryFeatureExtensions( - source.getID(), _feature, "supercluster", "children", {}); + getSource().getID(), _feature, "supercluster", "children", {}); if (featureExtension.is()) { return Feature::convert(env, featureExtension.get()); } @@ -131,7 +131,7 @@ jni::Local>> GeoJSONSource::getClusterL const std::map options = {{"limit", static_cast(limit)}, {"offset", static_cast(offset)}}; auto featureExtension = rendererFrontend->queryFeatureExtensions( - source.getID(), _feature, "supercluster", "leaves", options); + getSource().getID(), _feature, "supercluster", "leaves", options); if (featureExtension.is()) { return Feature::convert(env, featureExtension.get()); } @@ -148,7 +148,7 @@ jint GeoJSONSource::getClusterExpansionZoom(jni::JNIEnv& env, const jni::Object< mbgl::Feature _feature = Feature::convert(env, feature); _feature.properties["cluster_id"] = static_cast(_feature.properties["cluster_id"].get()); auto featureExtension = rendererFrontend->queryFeatureExtensions( - source.getID(), _feature, "supercluster", "expansion-zoom", {}); + getSource().getID(), _feature, "supercluster", "expansion-zoom", {}); if (featureExtension.is()) { auto value = featureExtension.get(); if (value.is()) { @@ -181,12 +181,20 @@ void GeoJSONSource::setAsync(Update::Converter converterFn) { awaitingUpdate = std::make_unique( std::move(converterFn), std::make_unique>( - *Scheduler::GetCurrent(), [this](std::shared_ptr geoJSONData) { + *Scheduler::GetCurrent(), + [awaitingUpdateWeak = std::weak_ptr(awaitingUpdate), + sourceWeak = getWeakSource(), + updateWeak = std::weak_ptr(update)](std::shared_ptr geoJSONData) { + auto awaitingUpdate = awaitingUpdateWeak.lock(); + auto source = sourceWeak.lock(); + auto update = updateWeak.lock(); + if (!awaitingUpdate || !source || !update) return; + // conversion from Java features to core ones finished android::UniqueEnv _env = android::AttachEnv(); // Update the core source - source.as()->setGeoJSONData(std::move(geoJSONData)); + source->get().as()->setGeoJSONData(std::move(geoJSONData)); // if there is an awaiting update, execute it, otherwise, release resources if (awaitingUpdate) { diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp index 6a360484b..ac4999bb5 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp @@ -70,8 +70,8 @@ private: jni::Local getURL(jni::JNIEnv&); jni::Local> createJavaPeer(jni::JNIEnv&); - std::unique_ptr awaitingUpdate; - std::unique_ptr update; + std::shared_ptr awaitingUpdate; + std::shared_ptr update; std::shared_ptr threadPool; std::unique_ptr> converter; diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/image_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/image_source.cpp index 8d278b318..f1f6559e4 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/image_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/image_source.cpp @@ -29,20 +29,20 @@ ImageSource::~ImageSource() = default; void ImageSource::setURL(jni::JNIEnv& env, const jni::String& url) { // Update the core source - source.as()->ImageSource::setURL(jni::Make(env, url)); + getSource().ImageSource::setURL(jni::Make(env, url)); } jni::Local ImageSource::getURL(jni::JNIEnv& env) { - std::optional url = source.as()->ImageSource::getURL(); + std::optional url = getSource().ImageSource::getURL(); return url ? jni::Make(env, *url) : jni::Local(); } void ImageSource::setImage(jni::JNIEnv& env, const jni::Object& bitmap) { - source.as()->setImage(Bitmap::GetImage(env, bitmap)); + getSource().setImage(Bitmap::GetImage(env, bitmap)); } void ImageSource::setCoordinates(jni::JNIEnv& env, const jni::Object& coordinatesObject) { - source.as()->setCoordinates(LatLngQuad::getLatLngArray(env, coordinatesObject)); + getSource().setCoordinates(LatLngQuad::getLatLngArray(env, coordinatesObject)); } jni::Local> ImageSource::createJavaPeer(jni::JNIEnv& env) { diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_dem_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_dem_source.cpp index 107c96a06..56bad65aa 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_dem_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_dem_source.cpp @@ -26,7 +26,7 @@ RasterDEMSource::RasterDEMSource(jni::JNIEnv& env, mbgl::style::Source& coreSour RasterDEMSource::~RasterDEMSource() = default; jni::Local RasterDEMSource::getURL(jni::JNIEnv& env) { - std::optional url = source.as()->RasterDEMSource::getURL(); + auto url = getSource().RasterDEMSource::getURL(); return url ? jni::Make(env, *url) : jni::Local(); } diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_source.cpp index d023fca6d..cba024237 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/raster_source.cpp @@ -25,7 +25,7 @@ RasterSource::RasterSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, An RasterSource::~RasterSource() = default; jni::Local RasterSource::getURL(jni::JNIEnv& env) { - std::optional url = source.as()->RasterSource::getURL(); + auto url = getSource().RasterSource::getURL(); return url ? jni::Make(env, *url) : jni::Local(); } diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/source.cpp index ec7026d44..af11e2a59 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/source.cpp @@ -70,13 +70,13 @@ Source::Source(jni::JNIEnv& env, mbgl::style::Source& coreSource, const jni::Object& obj, AndroidRendererFrontend* frontend) - : source(coreSource), + : source(std::make_shared>(coreSource)), javaPeer(jni::NewGlobal(env, obj)), rendererFrontend(frontend) {} Source::Source(jni::JNIEnv&, std::unique_ptr coreSource) : ownedSource(std::move(coreSource)), - source(*ownedSource) {} + source(std::make_shared>(*ownedSource)) {} Source::~Source() { if (ownedSource) { @@ -102,24 +102,24 @@ Source::~Source() { } jni::Local Source::getId(jni::JNIEnv& env) { - return jni::Make(env, source.getID()); + return jni::Make(env, source->get().getID()); } jni::Local Source::getAttribution(jni::JNIEnv& env) { - auto attribution = source.getAttribution(); + auto attribution = source->get().getAttribution(); return attribution ? jni::Make(env, attribution.value()) : jni::Make(env, ""); } void Source::setPrefetchZoomDelta(jni::JNIEnv& env, jni::Integer& delta) { if (!delta) { - source.setPrefetchZoomDelta(std::nullopt); + source->get().setPrefetchZoomDelta(std::nullopt); } else { - source.setPrefetchZoomDelta(jni::Unbox(env, delta)); + source->get().setPrefetchZoomDelta(jni::Unbox(env, delta)); } } jni::Local Source::getPrefetchZoomDelta(jni::JNIEnv& env) { - auto delta = source.getPrefetchZoomDelta(); + auto delta = source->get().getPrefetchZoomDelta(); if (delta.has_value()) { return jni::Box(env, jni::jint(delta.value())); } @@ -128,14 +128,14 @@ jni::Local Source::getPrefetchZoomDelta(jni::JNIEnv& env) { void Source::setMaxOverscaleFactorForParentTiles(jni::JNIEnv& env, jni::Integer& maxOverscaleFactor) { if (!maxOverscaleFactor) { - source.setMaxOverscaleFactorForParentTiles(std::nullopt); + source->get().setMaxOverscaleFactorForParentTiles(std::nullopt); } else { - source.setMaxOverscaleFactorForParentTiles(jni::Unbox(env, maxOverscaleFactor)); + source->get().setMaxOverscaleFactorForParentTiles(jni::Unbox(env, maxOverscaleFactor)); } } jni::Local Source::getMaxOverscaleFactorForParentTiles(jni::JNIEnv& env) { - auto maxOverscaleFactor = source.getMaxOverscaleFactorForParentTiles(); + auto maxOverscaleFactor = source->get().getMaxOverscaleFactorForParentTiles(); if (maxOverscaleFactor) { return jni::Box(env, jni::jint(*maxOverscaleFactor)); } @@ -151,7 +151,7 @@ void Source::addToStyle(JNIEnv& env, const jni::Object& obj, mbgl::style style.addSource(std::move(ownedSource)); // Add peer to core source - source.peer = std::unique_ptr(this); + source->get().peer = std::unique_ptr(this); // Add strong reference to java source javaPeer = jni::NewGlobal(env, obj); @@ -167,7 +167,7 @@ void Source::addToMap(JNIEnv& env, const jni::Object& obj, mbgl::Map& ma map.getStyle().addSource(std::move(ownedSource)); // Add peer to core source - source.peer = std::unique_ptr(this); + source->get().peer = std::unique_ptr(this); // Add strong reference to java source javaPeer = jni::NewGlobal(env, obj); @@ -182,26 +182,26 @@ bool Source::removeFromMap(JNIEnv&, const jni::Object&, mbgl::Map& map) } // Remove the source from the map and take ownership - ownedSource = map.getStyle().removeSource(source.getID()); + ownedSource = map.getStyle().removeSource(source->get().getID()); // The source may not be removed if any layers still reference it return ownedSource != nullptr; } jni::Local Source::isVolatile(jni::JNIEnv& env) { - return jni::Box(env, jni::jboolean(source.isVolatile())); + return jni::Box(env, jni::jboolean(source->get().isVolatile())); } void Source::setVolatile(JNIEnv& env, jni::Boolean& value) { - source.setVolatile(jni::Unbox(env, value)); + source->get().setVolatile(jni::Unbox(env, value)); } void Source::setMinimumTileUpdateInterval(JNIEnv& env, jni::Long& interval) { - source.setMinimumTileUpdateInterval(Milliseconds(jni::Unbox(env, interval))); + source->get().setMinimumTileUpdateInterval(Milliseconds(jni::Unbox(env, interval))); } jni::Local Source::getMinimumTileUpdateInterval(JNIEnv& env) { - return jni::Box(env, jni::jlong(source.getMinimumTileUpdateInterval().count() / 1000000)); + return jni::Box(env, jni::jlong(source->get().getMinimumTileUpdateInterval().count() / 1000000)); } void Source::releaseJavaPeer() { diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/source.hpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/source.hpp index b7782c348..8c9243f62 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/source.hpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/source.hpp @@ -8,6 +8,7 @@ #include "../../android_renderer_frontend.hpp" #include +#include namespace mbgl { namespace android { @@ -63,12 +64,21 @@ public: jni::Local getMinimumTileUpdateInterval(JNIEnv&); protected: + template + T& getSource() { + return *ownedSource->as(); + } + + std::weak_ptr> getWeakSource() const { return source; } + +private: // Set on newly created sources until added to the map. std::unique_ptr ownedSource; - // Raw pointer that is valid at all times. - mbgl::style::Source& source; + // Reference that is valid at all times. + std::shared_ptr> source; +protected: // Set when the source is added to a map. jni::Global> javaPeer; diff --git a/platform/android/MapLibreAndroid/src/cpp/style/sources/vector_source.cpp b/platform/android/MapLibreAndroid/src/cpp/style/sources/vector_source.cpp index 01e092f83..e3e77d57c 100644 --- a/platform/android/MapLibreAndroid/src/cpp/style/sources/vector_source.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/style/sources/vector_source.cpp @@ -31,7 +31,7 @@ VectorSource::VectorSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, An VectorSource::~VectorSource() = default; jni::Local VectorSource::getURL(jni::JNIEnv& env) { - std::optional url = source.as()->VectorSource::getURL(); + std::optional url = getSource().VectorSource::getURL(); return url ? jni::Make(env, *url) : jni::Local(); } @@ -42,7 +42,7 @@ jni::Local>> VectorSource::querySourceF std::vector features; if (rendererFrontend) { - features = rendererFrontend->querySourceFeatures(source.getID(), + features = rendererFrontend->querySourceFeatures(getSource().getID(), {toVector(env, jSourceLayerIds), toFilter(env, jfilter)}); } return Feature::convert(env, features); diff --git a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java index 5ab3e8ccc..04fbfc2a9 100644 --- a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java +++ b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java @@ -1,5 +1,6 @@ package org.maplibre.android.testapp.style; +import org.maplibre.android.style.sources.CannotAddSourceException; import org.maplibre.geojson.Feature; import org.maplibre.geojson.FeatureCollection; import org.maplibre.geojson.Point; @@ -155,6 +156,29 @@ public class GeoJsonSourceTests extends EspressoTest { }); } + @Test + public void testDuplicateSourceDuringAsyncSetGeoJson() { + // regression test for segfault when setting GeoJSON contents + // for a source that has been deleted + // https://github.com/maplibre/maplibre-native/issues/3493 + validateTestSetup(); + MapLibreMapAction.invoke(maplibreMap, (uiController, maplibreMap) -> { + String geoJsonString = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[0,0]}}"; + for (int i = 0; i < 1000; i++) { + // Create a GeoJSON source with the same ID each iteration + GeoJsonSource source = new GeoJsonSource("source"); + try { + maplibreMap.getStyle().addSource(source); + } catch (CannotAddSourceException ex) { + // ignore, this is expected + } + + // Schedule an async update via setGeoJson(String) + source.setGeoJson(geoJsonString); + } + }); + } + public abstract class BaseViewAction implements ViewAction { @Override