Skip to content

Commit

Permalink
Fix integer underflows when rounding up and input is 0
Browse files Browse the repository at this point in the history 
Fixes godotengine#80358

Whenever the code wants to round up, it performs x = (x - 1) / y + 1
when it should be doing x = (x + y - 1) / y

The original code when the input is 0 is wrong for both unsigned
(produces a very large value) and signed (produces 1 instead of 0).

This version handles 0 properly. (x / y = 0).

Mathematically they're the same:

= (x - 1) / y + 1
= (x - 1) / y + y / y
= (x + y - 1) / y

However in terms of computer science, they're not
  • Loading branch information
@darksylinc
darksylinc committed Sep 3, 2023
1 parent d2ae309 commit e826b64
Show file tree
Hide file tree
Showing 11 changed files with 52 additions and 34 deletions.
16 changes: 16 additions & 0 deletions core/math/math_funcs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,22 @@ class Math {
}
return p_target;
}

// Divides num / dem and the result is rounded up:
// Examples:
// 0 / 64 = 0
// 63 / 64 = 1
// 64 / 64 = 1
// 65 / 64 = 2
//
// Will not work correctly if numerator + denominator - 1u results in an overflow.
static _ALWAYS_INLINE_ uint32_t divide_round_up(uint32_t p_numerator, uint32_t p_denominator) {
return (p_numerator + p_denominator - 1u) / p_denominator;
}

static _ALWAYS_INLINE_ uint64_t divide_round_up(uint64_t p_numerator, uint64_t p_denominator) {
return (p_numerator + p_denominator - 1u) / p_denominator;
}
};

#endif // MATH_FUNCS_H
14 changes: 7 additions & 7 deletions drivers/gles3/storage/mesh_storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1347,7 +1347,7 @@ void MeshStorage::multimesh_set_mesh(RID p_multimesh, RID p_mesh) {
multimesh->dependency.changed_notify(Dependency::DEPENDENCY_CHANGED_MESH);
}

#define MULTIMESH_DIRTY_REGION_SIZE 512
#define MULTIMESH_DIRTY_REGION_SIZE 512u

void MeshStorage::_multimesh_make_local(MultiMesh *multimesh) const {
if (multimesh->data_cache.size() > 0 || multimesh->instances == 0) {
Expand All @@ -1371,7 +1371,7 @@ void MeshStorage::_multimesh_make_local(MultiMesh *multimesh) const {
memset(w, 0, (size_t)multimesh->instances * multimesh->stride_cache * sizeof(float));
}
}
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
multimesh->data_cache_dirty_regions = memnew_arr(bool, data_cache_dirty_region_count);
for (uint32_t i = 0; i < data_cache_dirty_region_count; i++) {
multimesh->data_cache_dirty_regions[i] = false;
Expand All @@ -1382,7 +1382,7 @@ void MeshStorage::_multimesh_make_local(MultiMesh *multimesh) const {
void MeshStorage::_multimesh_mark_dirty(MultiMesh *multimesh, int p_index, bool p_aabb) {
uint32_t region_index = p_index / MULTIMESH_DIRTY_REGION_SIZE;
#ifdef DEBUG_ENABLED
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
ERR_FAIL_UNSIGNED_INDEX(region_index, data_cache_dirty_region_count); //bug
#endif
if (!multimesh->data_cache_dirty_regions[region_index]) {
Expand All @@ -1403,7 +1403,7 @@ void MeshStorage::_multimesh_mark_dirty(MultiMesh *multimesh, int p_index, bool

void MeshStorage::_multimesh_mark_all_dirty(MultiMesh *multimesh, bool p_data, bool p_aabb) {
if (p_data) {
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);

for (uint32_t i = 0; i < data_cache_dirty_region_count; i++) {
if (!multimesh->data_cache_dirty_regions[i]) {
Expand Down Expand Up @@ -1750,7 +1750,7 @@ void MeshStorage::multimesh_set_buffer(RID p_multimesh, const Vector<float> &p_b
multimesh->data_cache = multimesh->data_cache;
{
//clear dirty since nothing will be dirty anymore
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
for (uint32_t i = 0; i < data_cache_dirty_region_count; i++) {
multimesh->data_cache_dirty_regions[i] = false;
}
Expand Down Expand Up @@ -1877,8 +1877,8 @@ void MeshStorage::_update_dirty_multimeshes() {
uint32_t visible_instances = multimesh->visible_instances >= 0 ? multimesh->visible_instances : multimesh->instances;

if (multimesh->data_cache_used_dirty_regions) {
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t visible_region_count = visible_instances == 0 ? 0 : (visible_instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
uint32_t visible_region_count = Math::divide_round_up(visible_instances, uint32_t(MULTIMESH_DIRTY_REGION_SIZE));

GLint region_size = multimesh->stride_cache * MULTIMESH_DIRTY_REGION_SIZE * sizeof(float);

Expand Down
4 changes: 3 additions & 1 deletion drivers/vulkan/rendering_device_vulkan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8208,7 +8208,9 @@ void RenderingDeviceVulkan::compute_list_dispatch_threads(ComputeListID p_list,

#endif

compute_list_dispatch(p_list, (p_x_threads - 1) / cl->state.local_group_size[0] + 1, (p_y_threads - 1) / cl->state.local_group_size[1] + 1, (p_z_threads - 1) / cl->state.local_group_size[2] + 1);
compute_list_dispatch(p_list, Math::divide_round_up(p_x_threads, cl->state.local_group_size[0]),
Math::divide_round_up(p_y_threads, cl->state.local_group_size[1]),
Math::divide_round_up(p_z_threads, cl->state.local_group_size[2]));
}

void RenderingDeviceVulkan::compute_list_dispatch_indirect(ComputeListID p_list, RID p_buffer, uint32_t p_offset) {
Expand Down
10 changes: 5 additions & 5 deletions modules/lightmapper_rd/lightmapper_rd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ LightmapperRD::BakeError LightmapperRD::_dilate(RenderingDevice *rd, Ref<RDShade
rd->compute_list_bind_uniform_set(compute_list, dilate_uniform_set, 1);
push_constant.region_ofs[0] = 0;
push_constant.region_ofs[1] = 0;
Vector3i group_size((atlas_size.x - 1) / 8 + 1, (atlas_size.y - 1) / 8 + 1, 1); //restore group size
Vector3i group_size(Math::divide_round_up(atlas_size.x, 8u), Math::divide_round_up(atlas_size.y, 8u), 1); //restore group size

for (int i = 0; i < atlas_slices; i++) {
push_constant.atlas_slice = i;
Expand Down Expand Up @@ -1036,7 +1036,7 @@ LightmapperRD::BakeError LightmapperRD::bake(BakeQuality p_quality, bool p_use_d
push_constant.environment_xform[11] = 0;
}

Vector3i group_size((atlas_size.x - 1) / 8 + 1, (atlas_size.y - 1) / 8 + 1, 1);
Vector3i group_size(Math::divide_round_up(atlas_size.x, 8u), Math::divide_round_up(atlas_size.y, 8u), 1);
rd->submit();
rd->sync();

Expand Down Expand Up @@ -1246,9 +1246,9 @@ LightmapperRD::BakeError LightmapperRD::bake(BakeQuality p_quality, bool p_use_d
int max_region_size = nearest_power_of_2_templated(int(GLOBAL_GET("rendering/lightmapping/bake_performance/region_size")));
int max_rays = GLOBAL_GET("rendering/lightmapping/bake_performance/max_rays_per_pass");

int x_regions = (atlas_size.width - 1) / max_region_size + 1;
int y_regions = (atlas_size.height - 1) / max_region_size + 1;
int ray_iterations = (push_constant.ray_count - 1) / max_rays + 1;
int x_regions = Math::divide_round_up(uint32_t(atlas_size.width), max_region_size);
int y_regions = Math::divide_round_up(uint32_t(atlas_size.height), max_region_size);
int ray_iterations = Math::divide_round_up(push_constant.ray_count, max_rays);

rd->submit();
rd->sync();
Expand Down
2 changes: 1 addition & 1 deletion scene/resources/bit_map.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ void BitMap::create(const Size2i &p_size) {

ERR_FAIL_COND(static_cast<int64_t>(p_size.width) * static_cast<int64_t>(p_size.height) > INT32_MAX);

Error err = bitmask.resize((((p_size.width * p_size.height) - 1) / 8) + 1);
Error err = bitmask.resize(Math::divide_round_up(p_size.width * p_size.height, 8u));
ERR_FAIL_COND(err != OK);

width = p_size.width;
Expand Down
6 changes: 3 additions & 3 deletions servers/rendering/renderer_rd/cluster_builder_rd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,8 @@ void ClusterBuilderRD::setup(Size2i p_screen_size, uint32_t p_max_elements, RID

screen_size = p_screen_size;

cluster_screen_size.width = (p_screen_size.width - 1) / cluster_size + 1;
cluster_screen_size.height = (p_screen_size.height - 1) / cluster_size + 1;
cluster_screen_size.width = Math::divide_round_up(p_screen_size.width, cluster_size);
cluster_screen_size.height = Math::divide_round_up(p_screen_size.height, cluster_size);

max_elements_by_type = p_max_elements;
if (max_elements_by_type % 32) { // Needs to be aligned to 32.
Expand Down Expand Up @@ -503,7 +503,7 @@ void ClusterBuilderRD::bake_cluster() {
push_constant.max_render_element_count_div_32 = render_element_max / 32;
push_constant.cluster_screen_size[0] = cluster_screen_size.x;
push_constant.cluster_screen_size[1] = cluster_screen_size.y;
push_constant.render_element_count_div_32 = render_element_count > 0 ? (render_element_count - 1) / 32 + 1 : 0;
push_constant.render_element_count_div_32 = Math::divide_round_up(render_element_count, 32u);
push_constant.max_cluster_element_count_div_32 = max_elements_by_type / 32;
push_constant.pad1 = 0;
push_constant.pad2 = 0;
Expand Down
4 changes: 2 additions & 2 deletions servers/rendering/renderer_rd/effects/copy_effects.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1018,8 +1018,8 @@ void CopyEffects::cubemap_downsample(RID p_source_cubemap, RID p_dest_cubemap, c
RD::get_singleton()->compute_list_bind_uniform_set(compute_list, uniform_set_cache->get_cache(shader, 0, u_source_cubemap), 0);
RD::get_singleton()->compute_list_bind_uniform_set(compute_list, uniform_set_cache->get_cache(shader, 1, u_dest_cubemap), 1);

int x_groups = (p_size.x - 1) / 8 + 1;
int y_groups = (p_size.y - 1) / 8 + 1;
int x_groups = Math::divide_round_up(p_size.x, 8u);
int y_groups = Math::divide_round_up(p_size.y, 8u);

RD::get_singleton()->compute_list_set_push_constant(compute_list, &cubemap_downsampler.push_constant, sizeof(CubemapDownsamplerPushConstant));

Expand Down
4 changes: 2 additions & 2 deletions servers/rendering/renderer_rd/environment/fog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1069,8 +1069,8 @@ void Fog::volumetric_fog_update(const VolumetricFogSettings &p_settings, const P
uint32_t cluster_size = p_settings.cluster_builder->get_cluster_size();
params.cluster_shift = get_shift_from_power_of_2(cluster_size);

uint32_t cluster_screen_width = (p_settings.rb_size.x - 1) / cluster_size + 1;
uint32_t cluster_screen_height = (p_settings.rb_size.y - 1) / cluster_size + 1;
uint32_t cluster_screen_width = Math::divide_round_up(p_settings.rb_size.x, cluster_size);
uint32_t cluster_screen_height = Math::divide_round_up(p_settings.rb_size.y, cluster_size);
params.max_cluster_element_count_div_32 = p_settings.max_cluster_elements / 32;
params.cluster_type_size = cluster_screen_width * cluster_screen_height * (params.max_cluster_element_count_div_32 + 32);
params.cluster_width = cluster_screen_width;
Expand Down
8 changes: 4 additions & 4 deletions servers/rendering/renderer_rd/environment/gi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2966,7 +2966,7 @@ void GI::VoxelGIInstance::update(bool p_update_light_instances, const Vector<RID
push_constant.cell_offset = mipmaps[i].cell_offset;
push_constant.cell_count = mipmaps[i].cell_count;

int64_t wg_todo = (mipmaps[i].cell_count + wg_size - 1) / wg_size;
int64_t wg_todo = Math::divide_round_up(mipmaps[i].cell_count, wg_size);
while (wg_todo) {
int64_t wg_count = MIN(wg_todo, wg_limit_x);
RD::get_singleton()->compute_list_set_push_constant(compute_list, &push_constant, sizeof(VoxelGIPushConstant));
Expand All @@ -2987,7 +2987,7 @@ void GI::VoxelGIInstance::update(bool p_update_light_instances, const Vector<RID
push_constant.cell_offset = mipmaps[i].cell_offset;
push_constant.cell_count = mipmaps[i].cell_count;

int64_t wg_todo = (mipmaps[i].cell_count + wg_size - 1) / wg_size;
int64_t wg_todo = Math::divide_round_up(mipmaps[i].cell_count, wg_size);
while (wg_todo) {
int64_t wg_count = MIN(wg_todo, wg_limit_x);
RD::get_singleton()->compute_list_set_push_constant(compute_list, &push_constant, sizeof(VoxelGIPushConstant));
Expand Down Expand Up @@ -3138,7 +3138,7 @@ void GI::VoxelGIInstance::update(bool p_update_light_instances, const Vector<RID
RD::get_singleton()->compute_list_bind_compute_pipeline(compute_list, gi->voxel_gi_lighting_shader_version_pipelines[VOXEL_GI_SHADER_VERSION_DYNAMIC_OBJECT_LIGHTING]);
RD::get_singleton()->compute_list_bind_uniform_set(compute_list, dynamic_maps[0].uniform_set, 0);
RD::get_singleton()->compute_list_set_push_constant(compute_list, &push_constant, sizeof(VoxelGIDynamicPushConstant));
RD::get_singleton()->compute_list_dispatch(compute_list, (rect.size.x - 1) / 8 + 1, (rect.size.y - 1) / 8 + 1, 1);
RD::get_singleton()->compute_list_dispatch(compute_list, Math::divide_round_up(rect.size.x, 8u), Math::divide_round_up(rect.size.y, 8u), 1);
//print_line("rect: " + itos(i) + ": " + rect);

for (int k = 1; k < dynamic_maps.size(); k++) {
Expand Down Expand Up @@ -3200,7 +3200,7 @@ void GI::VoxelGIInstance::update(bool p_update_light_instances, const Vector<RID
}
RD::get_singleton()->compute_list_bind_uniform_set(compute_list, dynamic_maps[k].uniform_set, 0);
RD::get_singleton()->compute_list_set_push_constant(compute_list, &push_constant, sizeof(VoxelGIDynamicPushConstant));
RD::get_singleton()->compute_list_dispatch(compute_list, (rect.size.x - 1) / 8 + 1, (rect.size.y - 1) / 8 + 1, 1);
RD::get_singleton()->compute_list_dispatch(compute_list, Math::divide_round_up(rect.size.x, 8u), Math::divide_round_up(rect.size.y, 8u), 1);
}

RD::get_singleton()->compute_list_end();
Expand Down
4 changes: 2 additions & 2 deletions servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -605,8 +605,8 @@ void RenderForwardClustered::_setup_environment(const RenderDataRD *p_render_dat
scene_state.ubo.cluster_shift = get_shift_from_power_of_2(p_render_data->cluster_size);
scene_state.ubo.max_cluster_element_count_div_32 = p_render_data->cluster_max_elements / 32;
{
uint32_t cluster_screen_width = (p_screen_size.width - 1) / p_render_data->cluster_size + 1;
uint32_t cluster_screen_height = (p_screen_size.height - 1) / p_render_data->cluster_size + 1;
uint32_t cluster_screen_width = Math::divide_round_up(p_screen_size.width, p_render_data->cluster_size);
uint32_t cluster_screen_height = Math::divide_round_up(p_screen_size.height, p_render_data->cluster_size);
scene_state.ubo.cluster_type_size = cluster_screen_width * cluster_screen_height * (scene_state.ubo.max_cluster_element_count_div_32 + 32);
scene_state.ubo.cluster_width = cluster_screen_width;
}
Expand Down
14 changes: 7 additions & 7 deletions servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,7 @@ void MeshStorage::multimesh_set_mesh(RID p_multimesh, RID p_mesh) {
multimesh->dependency.changed_notify(Dependency::DEPENDENCY_CHANGED_MESH);
}

#define MULTIMESH_DIRTY_REGION_SIZE 512
#define MULTIMESH_DIRTY_REGION_SIZE 512u

void MeshStorage::_multimesh_make_local(MultiMesh *multimesh) const {
if (multimesh->data_cache.size() > 0) {
Expand All @@ -1459,7 +1459,7 @@ void MeshStorage::_multimesh_make_local(MultiMesh *multimesh) const {
memset(w, 0, buffer_size * sizeof(float));
}
}
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
multimesh->data_cache_dirty_regions = memnew_arr(bool, data_cache_dirty_region_count);
memset(multimesh->data_cache_dirty_regions, 0, data_cache_dirty_region_count * sizeof(bool));
multimesh->data_cache_dirty_region_count = 0;
Expand Down Expand Up @@ -1487,7 +1487,7 @@ void MeshStorage::_multimesh_update_motion_vectors_data_cache(MultiMesh *multime
uint32_t current_ofs = multimesh->motion_vectors_current_offset * multimesh->stride_cache * sizeof(float);
uint32_t previous_ofs = multimesh->motion_vectors_previous_offset * multimesh->stride_cache * sizeof(float);
uint32_t visible_instances = multimesh->visible_instances >= 0 ? multimesh->visible_instances : multimesh->instances;
uint32_t visible_region_count = visible_instances == 0 ? 0 : (visible_instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t visible_region_count = Math::divide_round_up(visible_instances, MULTIMESH_DIRTY_REGION_SIZE);
uint32_t region_size = multimesh->stride_cache * MULTIMESH_DIRTY_REGION_SIZE * sizeof(float);
uint32_t size = multimesh->stride_cache * (uint32_t)multimesh->instances * (uint32_t)sizeof(float);
for (uint32_t i = 0; i < visible_region_count; i++) {
Expand All @@ -1503,7 +1503,7 @@ void MeshStorage::_multimesh_update_motion_vectors_data_cache(MultiMesh *multime
void MeshStorage::_multimesh_mark_dirty(MultiMesh *multimesh, int p_index, bool p_aabb) {
uint32_t region_index = p_index / MULTIMESH_DIRTY_REGION_SIZE;
#ifdef DEBUG_ENABLED
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
ERR_FAIL_UNSIGNED_INDEX(region_index, data_cache_dirty_region_count); //bug
#endif
if (!multimesh->data_cache_dirty_regions[region_index]) {
Expand All @@ -1524,7 +1524,7 @@ void MeshStorage::_multimesh_mark_dirty(MultiMesh *multimesh, int p_index, bool

void MeshStorage::_multimesh_mark_all_dirty(MultiMesh *multimesh, bool p_data, bool p_aabb) {
if (p_data) {
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);

for (uint32_t i = 0; i < data_cache_dirty_region_count; i++) {
if (!multimesh->data_cache_dirty_regions[i]) {
Expand Down Expand Up @@ -1923,8 +1923,8 @@ void MeshStorage::_update_dirty_multimeshes() {

uint32_t total_dirty_regions = multimesh->data_cache_dirty_region_count + multimesh->previous_data_cache_dirty_region_count;
if (total_dirty_regions != 0) {
uint32_t data_cache_dirty_region_count = (multimesh->instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t visible_region_count = visible_instances == 0 ? 0 : (visible_instances - 1) / MULTIMESH_DIRTY_REGION_SIZE + 1;
uint32_t data_cache_dirty_region_count = Math::divide_round_up(multimesh->instances, MULTIMESH_DIRTY_REGION_SIZE);
uint32_t visible_region_count = Math::divide_round_up(visible_instances, MULTIMESH_DIRTY_REGION_SIZE);

uint32_t region_size = multimesh->stride_cache * MULTIMESH_DIRTY_REGION_SIZE * sizeof(float);
if (total_dirty_regions > 32 || total_dirty_regions > visible_region_count / 2) {
Expand Down

0 comments on commit e826b64

Please sign in to comment.