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

Tighten size check #8562

Merged
merged 6 commits into from
Apr 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/mbgl/util/image.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class Image : private util::noncopyable {
}

bool valid() const {
return size && data.get() != nullptr;
return !size.isEmpty() && data.get() != nullptr;
}

template <typename T = Image>
Expand Down
11 changes: 5 additions & 6 deletions include/mbgl/util/size.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ namespace mbgl {

class Size {
public:
constexpr Size() : width(0), height(0) {
}
constexpr Size() = default;

constexpr Size(const uint32_t width_, const uint32_t height_) : width(width_), height(height_) {
}
Expand All @@ -17,12 +16,12 @@ class Size {
return width * height;
}

constexpr explicit operator bool() const {
return width > 0 && height > 0;
constexpr bool isEmpty() const {
return width == 0 || height == 0;
}

uint32_t width;
uint32_t height;
uint32_t width = 0;
uint32_t height = 0;
};

constexpr inline bool operator==(const Size& a, const Size& b) {
Expand Down
2 changes: 1 addition & 1 deletion platform/android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ buildscript {
jcenter()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.0'
classpath 'com.android.tools.build:gradle:2.3.1'
classpath 'com.amazonaws:aws-devicefarm-gradle-plugin:1.2'
classpath 'com.stanfy.spoon:spoon-gradle-plugin:1.2.1'
}
Expand Down
12 changes: 10 additions & 2 deletions platform/android/src/native_map_view.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <jni/jni.hpp>

#include <mbgl/map/backend_scope.hpp>
#include <mbgl/math/minmax.hpp>
#include <mbgl/util/constants.hpp>
#include <mbgl/util/event.hpp>
#include <mbgl/util/exception.hpp>
Expand Down Expand Up @@ -72,6 +73,10 @@ NativeMapView::NativeMapView(jni::JNIEnv& _env,
MapMode::Continuous, GLContextMode::Unique, ConstrainMode::HeightOnly,
ViewportMode::Default, jni::Make<std::string>(_env, _programCacheDir));

recalculateSourceTileCacheSize();
}

void NativeMapView::recalculateSourceTileCacheSize() {
//Calculate a fitting cache size based on device parameters
float zoomFactor = map->getMaxZoom() - map->getMinZoom() + 1;
float cpuFactor = availableProcessors;
Expand Down Expand Up @@ -320,9 +325,10 @@ void NativeMapView::update(jni::JNIEnv&) {
}

void NativeMapView::resizeView(jni::JNIEnv&, int w, int h) {
width = w;
height = h;
width = util::max(64, w);
height = util::max(64, h);
map->setSize({ static_cast<uint32_t>(width), static_cast<uint32_t>(height) });
recalculateSourceTileCacheSize();
}

void NativeMapView::resizeFramebuffer(jni::JNIEnv&, int w, int h) {
Expand Down Expand Up @@ -479,6 +485,7 @@ void NativeMapView::resetZoom(jni::JNIEnv&) {

void NativeMapView::setMinZoom(jni::JNIEnv&, jni::jdouble zoom) {
map->setMinZoom(zoom);
recalculateSourceTileCacheSize();
}

jni::jdouble NativeMapView::getMinZoom(jni::JNIEnv&) {
Expand All @@ -487,6 +494,7 @@ jni::jdouble NativeMapView::getMinZoom(jni::JNIEnv&) {

void NativeMapView::setMaxZoom(jni::JNIEnv&, jni::jdouble zoom) {
map->setMaxZoom(zoom);
recalculateSourceTileCacheSize();
}

jni::jdouble NativeMapView::getMaxZoom(jni::JNIEnv&) {
Expand Down
11 changes: 7 additions & 4 deletions platform/android/src/native_map_view.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ class NativeMapView : public View, public Backend {
void updateFps();

private:
void recalculateSourceTileCacheSize();

JavaVM *vm = nullptr;
jni::UniqueWeakObject<NativeMapView> javaPeer;
Expand Down Expand Up @@ -315,10 +316,12 @@ class NativeMapView : public View, public Backend {
bool firstRender = true;
double fps = 0.0;

int width = 0;
int height = 0;
int fbWidth = 0;
int fbHeight = 0;
// Minimum texture size according to OpenGL ES 2.0 specification.
int width = 64;
int height = 64;
int fbWidth = 64;
int fbHeight = 64;

bool framebufferSizeChanged = true;

int availableProcessors = 0;
Expand Down
2 changes: 1 addition & 1 deletion platform/default/mbgl/gl/offscreen_view.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace mbgl {
class OffscreenView::Impl {
public:
Impl(gl::Context& context_, const Size size_) : context(context_), size(std::move(size_)) {
assert(size);
assert(!size.isEmpty());
}

void bind() {
Expand Down
7 changes: 5 additions & 2 deletions platform/ios/src/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -576,8 +576,11 @@ - (void)commonInit

- (mbgl::Size)size
{
return { static_cast<uint32_t>(self.bounds.size.width),
static_cast<uint32_t>(self.bounds.size.height) };
// check for minimum texture size supported by OpenGL ES 2.0
//
CGSize size = CGSizeMake(MAX(self.bounds.size.width, 64), MAX(self.bounds.size.height, 64));
return { static_cast<uint32_t>(size.width),
static_cast<uint32_t>(size.height) };
}

- (mbgl::Size)framebufferSize
Expand Down
2 changes: 1 addition & 1 deletion platform/ios/test/MGLAnnotationViewTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ @implementation MGLAnnotationViewTests
- (void)setUp
{
[super setUp];
_mapView = [[MGLMapView alloc] initWithFrame:CGRectZero];
_mapView = [[MGLMapView alloc] initWithFrame:CGRectMake(0, 0, 64, 64)];
_mapView.delegate = self;
}

Expand Down
7 changes: 5 additions & 2 deletions platform/macos/src/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,11 @@ - (void)commonInit {
}

- (mbgl::Size)size {
return { static_cast<uint32_t>(self.bounds.size.width),
static_cast<uint32_t>(self.bounds.size.height) };
// check for minimum texture size supported by OpenGL ES 2.0
//
CGSize size = CGSizeMake(MAX(self.bounds.size.width, 64), MAX(self.bounds.size.height, 64));
return { static_cast<uint32_t>(size.width),
static_cast<uint32_t>(size.height) };
}

- (mbgl::Size)framebufferSize {
Expand Down
30 changes: 20 additions & 10 deletions platform/qt/src/qmapboxgl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <mbgl/map/camera.hpp>
#include <mbgl/map/map.hpp>
#include <mbgl/map/backend_scope.hpp>
#include <mbgl/math/minmax.hpp>
#include <mbgl/style/conversion.hpp>
#include <mbgl/style/conversion/layer.hpp>
#include <mbgl/style/conversion/source.hpp>
Expand Down Expand Up @@ -79,6 +80,13 @@ auto fromQStringList(const QStringList &list)
return strings;
}

mbgl::Size sanitizedSize(const QSize& size) {
return mbgl::Size {
mbgl::util::max(0u, static_cast<uint32_t>(size.width())),
mbgl::util::max(0u, static_cast<uint32_t>(size.height())),
};
};

std::unique_ptr<const mbgl::SpriteImage> toSpriteImage(const QImage &sprite) {
const QImage swapped = sprite
.rgbSwapped()
Expand Down Expand Up @@ -433,6 +441,8 @@ void QMapboxGLSettings::setApiBaseUrl(const QString& url)
QMapboxGL::QMapboxGL(QObject *parent, const QMapboxGLSettings &settings, const QSize& size, qreal pixelRatio)
: QObject(parent)
{
assert(!size.isEmpty());

// Multiple QMapboxGL running on the same thread
// will share the same mbgl::util::RunLoop
if (!loop.hasLocalData()) {
Expand Down Expand Up @@ -1079,8 +1089,7 @@ void QMapboxGL::resize(const QSize& size, const QSize& framebufferSize)
d_ptr->size = size;
d_ptr->fbSize = framebufferSize;

d_ptr->mapObj->setSize(
{ static_cast<uint32_t>(size.width()), static_cast<uint32_t>(size.height()) });
d_ptr->mapObj->setSize(sanitizedSize(size));
}

/*!
Expand Down Expand Up @@ -1547,14 +1556,15 @@ QMapboxGLPrivate::QMapboxGLPrivate(QMapboxGL *q, const QMapboxGLSettings &settin
settings.assetPath().toStdString(),
settings.cacheDatabaseMaximumSize()))
, threadPool(mbgl::sharedThreadPool())
, mapObj(std::make_unique<mbgl::Map>(
*this, mbgl::Size{ static_cast<uint32_t>(size.width()), static_cast<uint32_t>(size.height()) },
pixelRatio, *fileSourceObj, *threadPool,
mbgl::MapMode::Continuous,
static_cast<mbgl::GLContextMode>(settings.contextMode()),
static_cast<mbgl::ConstrainMode>(settings.constrainMode()),
static_cast<mbgl::ViewportMode>(settings.viewportMode())))
{
mapObj = std::make_unique<mbgl::Map>(
*this, sanitizedSize(size),
Copy link
Member

Choose a reason for hiding this comment

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

why is size invalid here and needs to be sanitized in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because QSize default constructor initializes witdh and height with -1. This causes an overflow when converting from int to uint32_t- using std::numeric_limits<uint32_t>::max() for width and height has proven to cause undefined behavior in places like tileCover.

Qt sometimes initializes a dummy map plugin as means of discovering which map plugins it has available - when it does, it uses a default QSize for initializing the item size.

pixelRatio, *fileSourceObj, *threadPool,
mbgl::MapMode::Continuous,
static_cast<mbgl::GLContextMode>(settings.contextMode()),
static_cast<mbgl::ConstrainMode>(settings.constrainMode()),
static_cast<mbgl::ViewportMode>(settings.viewportMode()));

qRegisterMetaType<QMapboxGL::MapChange>("QMapboxGL::MapChange");

fileSourceObj->setAccessToken(settings.accessToken().toStdString());
Expand All @@ -1570,7 +1580,7 @@ QMapboxGLPrivate::~QMapboxGLPrivate()
}

mbgl::Size QMapboxGLPrivate::framebufferSize() const {
return { static_cast<uint32_t>(fbSize.width()), static_cast<uint32_t>(fbSize.height()) };
return sanitizedSize(fbSize);
}

void QMapboxGLPrivate::updateAssumedState() {
Expand Down
10 changes: 6 additions & 4 deletions src/mbgl/map/transform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,13 @@ Transform::Transform(MapObserver& observer_,

#pragma mark - Map View

bool Transform::resize(const Size size) {
void Transform::resize(const Size size) {
if (size.isEmpty()) {
throw std::runtime_error("failed to resize: size is empty");
}

if (state.size == size) {
return false;
return;
}

observer.onCameraWillChange(MapObserver::CameraChangeMode::Immediate);
Expand All @@ -56,8 +60,6 @@ bool Transform::resize(const Size size) {
state.constrain(state.scale, state.x, state.y);

observer.onCameraDidChange(MapObserver::CameraChangeMode::Immediate);

return true;
}

#pragma mark - Camera
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/map/transform.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class Transform : private util::noncopyable {
Transform(const TransformState &state_) : observer(MapObserver::nullObserver()), state(state_) {}

// Map view
bool resize(Size size);
void resize(Size size);

// Camera
/** Returns the current camera options. */
Expand Down
7 changes: 5 additions & 2 deletions src/mbgl/map/transform_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ void TransformState::matrixFor(mat4& matrix, const UnwrappedTileID& tileID) cons
}

void TransformState::getProjMatrix(mat4& projMatrix) const {
if (size.isEmpty()) {
return;
}

// Find the distance from the center point [width/2, height/2] to the
// center top point [width/2, 0] in Z units, using the law of sines.
Expand Down Expand Up @@ -216,7 +219,7 @@ double TransformState::scaleZoom(double s) const {
}

ScreenCoordinate TransformState::latLngToScreenCoordinate(const LatLng& latLng) const {
if (!size) {
if (size.isEmpty()) {
return {};
}

Expand All @@ -229,7 +232,7 @@ ScreenCoordinate TransformState::latLngToScreenCoordinate(const LatLng& latLng)
}

LatLng TransformState::screenCoordinateToLatLng(const ScreenCoordinate& point, LatLng::WrapMode wrapMode) const {
if (!size) {
if (size.isEmpty()) {
return {};
}

Expand Down
4 changes: 4 additions & 0 deletions src/mbgl/map/transform_state.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ class TransformState {
double zoomScale(double zoom) const;
double scaleZoom(double scale) const;

bool valid() const {
return !size.isEmpty() && (scale >= min_scale && scale <= max_scale);
}

private:
bool rotatedNorth() const;
void constrain(double& scale, double& x, double& y) const;
Expand Down
4 changes: 2 additions & 2 deletions src/mbgl/util/offscreen_texture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace mbgl {
class OffscreenTexture::Impl {
public:
Impl(gl::Context& context_, const Size size_) : context(context_), size(std::move(size_)) {
assert(size);
assert(!size.isEmpty());
}

void bind() {
Expand Down Expand Up @@ -45,7 +45,7 @@ class OffscreenTexture::Impl {

OffscreenTexture::OffscreenTexture(gl::Context& context, const Size size)
: impl(std::make_unique<Impl>(context, std::move(size))) {
assert(size);
assert(!size.isEmpty());
}

OffscreenTexture::~OffscreenTexture() = default;
Expand Down
2 changes: 2 additions & 0 deletions src/mbgl/util/tile_cover.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ std::vector<UnwrappedTileID> tileCover(const LatLngBounds& bounds_, int32_t z) {
}

std::vector<UnwrappedTileID> tileCover(const TransformState& state, int32_t z) {
assert(state.valid());

const double w = state.getSize().width;
const double h = state.getSize().height;
return tileCover(
Expand Down
Loading