From 86cebd4ccae1d0c5702ad75f1b0fb0d634bb8680 Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Tue, 12 Jul 2016 15:44:36 +0300 Subject: [core] Report conversion errors using std::string char* increases the risk of pointing to a invalid reference. Qt had to use a static variable as retainer to workaround. --- include/mbgl/style/conversion.hpp | 4 +++- platform/node/src/node_map.cpp | 10 +++++----- platform/qt/src/qmapboxgl.cpp | 6 +++--- platform/qt/src/qt_geojson.hpp | 10 ++-------- src/mbgl/style/sources/geojson_source_impl.cpp | 2 +- 5 files changed, 14 insertions(+), 18 deletions(-) diff --git a/include/mbgl/style/conversion.hpp b/include/mbgl/style/conversion.hpp index bd7db3adfb..e53adcb942 100644 --- a/include/mbgl/style/conversion.hpp +++ b/include/mbgl/style/conversion.hpp @@ -2,6 +2,8 @@ #include +#include + namespace mbgl { namespace style { namespace conversion { @@ -53,7 +55,7 @@ namespace conversion { them for v8 types. */ -struct Error { const char * message; }; +struct Error { std::string message; }; template class Result : private variant { diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp index 24a1215ea8..77d650e2e9 100644 --- a/platform/node/src/node_map.cpp +++ b/platform/node/src/node_map.cpp @@ -504,7 +504,7 @@ NAN_METHOD(NodeMap::AddSource) { Result> source = convert>(info[1], *Nan::Utf8String(info[0])); if (!source) { - Nan::ThrowTypeError(source.error().message); + Nan::ThrowTypeError(source.error().message.c_str()); return; } @@ -524,7 +524,7 @@ NAN_METHOD(NodeMap::AddLayer) { Result> layer = convert>(info[0]); if (!layer) { - Nan::ThrowTypeError(layer.error().message); + Nan::ThrowTypeError(layer.error().message.c_str()); return; } @@ -557,7 +557,7 @@ NAN_METHOD(NodeMap::SetLayoutProperty) { mbgl::optional error = setLayoutProperty(*layer, *Nan::Utf8String(info[1]), info[2]); if (error) { - return Nan::ThrowTypeError(error->message); + return Nan::ThrowTypeError(error->message.c_str()); } nodeMap->map->update(mbgl::Update::RecalculateStyle); @@ -595,7 +595,7 @@ NAN_METHOD(NodeMap::SetPaintProperty) { mbgl::optional error = setPaintProperty(*layer, *Nan::Utf8String(info[1]), info[2], klass); if (error) { - return Nan::ThrowTypeError(error->message); + return Nan::ThrowTypeError(error->message.c_str()); } nodeMap->map->update(mbgl::Update::RecalculateStyle | mbgl::Update::Classes); @@ -627,7 +627,7 @@ NAN_METHOD(NodeMap::SetFilter) { if (!info[1]->IsNull() && !info[1]->IsUndefined()) { Result converted = convert(info[1]); if (!converted) { - Nan::ThrowTypeError(converted.error().message); + Nan::ThrowTypeError(converted.error().message.c_str()); return; } filter = std::move(*converted); diff --git a/platform/qt/src/qmapboxgl.cpp b/platform/qt/src/qmapboxgl.cpp index 71264faed7..d9eb43cf05 100644 --- a/platform/qt/src/qmapboxgl.cpp +++ b/platform/qt/src/qmapboxgl.cpp @@ -628,7 +628,7 @@ void QMapboxGL::addSource(const QString& sourceID, const QVariant& value) Result> source = convert>(value, sourceID.toStdString()); if (!source) { - qWarning() << "Unable to add source:" << source.error().message; + qWarning() << "Unable to add source:" << source.error().message.c_str(); return; } @@ -665,7 +665,7 @@ void QMapboxGL::addLayer(const QVariant& value) Result> layer = convert>(value); if (!layer) { - qWarning() << "Unable to add layer:" << layer.error().message; + qWarning() << "Unable to add layer:" << layer.error().message.c_str(); return; } @@ -692,7 +692,7 @@ void QMapboxGL::setFilter(const QString& layer_, const QVariant& filter_) Result converted = convert(filter_); if (!converted) { - qWarning() << "Error parsing filter:" << converted.error().message; + qWarning() << "Error parsing filter:" << converted.error().message.c_str(); return; } filter = std::move(*converted); diff --git a/platform/qt/src/qt_geojson.hpp b/platform/qt/src/qt_geojson.hpp index ff7ee2eada..fd2b689fed 100644 --- a/platform/qt/src/qt_geojson.hpp +++ b/platform/qt/src/qt_geojson.hpp @@ -29,22 +29,16 @@ Result convertGeoJSON(const QVariant& value) { d.Parse<0>(value.toByteArray().constData()); } - // Needed to keep the error message alive - // when we go out of this scope. - static std::string error; - if (d.HasParseError()) { std::stringstream message; message << d.GetErrorOffset() << " - " << rapidjson::GetParseError_En(d.GetParseError()); - error = message.str(); - return Error { error.c_str() }; + return Error { message.str() }; } conversion::Result geoJSON = conversion::convertGeoJSON(d); if (!geoJSON) { - error = geoJSON.error().message; - return Error { error.c_str() }; + return Error { geoJSON.error().message }; } return geoJSON; diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index d71df28378..9e6b3d34f6 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -83,7 +83,7 @@ void GeoJSONSource::Impl::load(FileSource& fileSource) { conversion::Result geoJSON = conversion::convertGeoJSON(d); if (!geoJSON) { - Log::Error(Event::ParseStyle, "Failed to parse GeoJSON data: %s", geoJSON.error().message); + Log::Error(Event::ParseStyle, "Failed to parse GeoJSON data: %s", geoJSON.error().message.c_str()); // Create an empty GeoJSON VT object to make sure we're not infinitely waiting for // tiles to load. mapbox::geojson::feature_collection features; -- cgit v1.2.1