From 3d401e71452d890eaf0bc50b11788cb08a6c2fed Mon Sep 17 00:00:00 2001
From: "Benjamin A. Beasley" <code@musicinmybrain.net>
Date: Tue, 17 Aug 2021 21:30:44 -0400
Subject: [PATCH] =?UTF-8?q?Fix=20undefined=20behavior=20from=20array=20?=
 =?UTF-8?q?=E2=80=9Cshape-punning=E2=80=9D?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In stb_voxel_render.h, there were three cases where a 2D array of
dimension [X][Y] was iterated as a 1D array of dimension [1][X*Y]. While
this is clever and is correct in terms of the actual memory layout, a
second index outside the corresponding dimension ([i][j], j >= Y])
actually produces undefined behavior and gives the compiler freedom to
do all sorts of terrible things.

The same thing happens in stb_tilemap_editor.h,
tests/caveview/cave_mesher.c, and tests/resample_test.cpp.

Prior to this commit, a compiler warning regarding the undefined
behavior appears on gcc 11.2.1 for at least some of these cases when the
tests are compiled with -Waggressive-loop-optimizations (included in
-Wall).

This commit fixes the undefined behavior by iterating these 2D arrays
with the conventional nested loops.
---
 stb_tilemap_editor.h         | 35 +++++++++++++++++++----------------
 stb_voxel_render.h           | 36 +++++++++++++++++++++---------------
 tests/caveview/cave_mesher.c | 26 ++++++++++++++------------
 tests/resample_test.cpp      | 15 +++++++++------
 4 files changed, 63 insertions(+), 49 deletions(-)

diff --git a/stb_tilemap_editor.h b/stb_tilemap_editor.h
index fbd3388084..0b8c2ca997 100644
--- a/stb_tilemap_editor.h
+++ b/stb_tilemap_editor.h
@@ -1066,14 +1066,15 @@ stbte_tilemap *stbte_create_map(int map_x, int map_y, int map_layers, int spacin
 
 void stbte_set_background_tile(stbte_tilemap *tm, short id)
 {
-   int i;
+   int i, j;
    STBTE_ASSERT(id >= -1);
    // STBTE_ASSERT(id < 32768);
    if (id < -1)
       return;
-   for (i=0; i < STBTE_MAX_TILEMAP_X * STBTE_MAX_TILEMAP_Y; ++i)
-      if (tm->data[0][i][0] == -1)
-         tm->data[0][i][0] = id;
+   for (i=0; i < STBTE_MAX_TILEMAP_X; ++i)
+      for (j=0; j < STBTE_MAX_TILEMAP_Y; ++j)
+        if (tm->data[i][j][0] == -1)
+           tm->data[i][j][0] = id;
    tm->background_tile = id;
 }
 
@@ -1212,18 +1213,20 @@ void stbte_set_dimensions(stbte_tilemap *tm, int map_x, int map_y)
 
 void stbte_clear_map(stbte_tilemap *tm)
 {
-   int i,j;
-   for (i=0; i < STBTE_MAX_TILEMAP_X * STBTE_MAX_TILEMAP_Y; ++i) {
-      tm->data[0][i][0] = tm->background_tile;
-      for (j=1; j < tm->num_layers; ++j)
-         tm->data[0][i][j] = STBTE__NO_TILE;
-      for (j=0; j < STBTE_MAX_PROPERTIES; ++j)
-         tm->props[0][i][j] = 0;
-      #ifdef STBTE_ALLOW_LINK
-      tm->link[0][i].x = -1;
-      tm->link[0][i].y = -1;
-      tm->linkcount[0][i] = 0;
-      #endif
+   int i,j,k;
+   for (i=0; i < STBTE_MAX_TILEMAP_X; ++i) {
+      for (j=0; j < STBTE_MAX_TILEMAP_Y; ++j) {
+         tm->data[i][j][0] = tm->background_tile;
+         for (k=1; k < tm->num_layers; ++k)
+            tm->data[i][j][k] = STBTE__NO_TILE;
+         for (k=0; k < STBTE_MAX_PROPERTIES; ++k)
+            tm->props[i][j][k] = 0;
+         #ifdef STBTE_ALLOW_LINK
+         tm->link[i][j].x = -1;
+         tm->link[i][j].y = -1;
+         tm->linkcount[i][j] = 0;
+         #endif
+      }
    }
 }
 
diff --git a/stb_voxel_render.h b/stb_voxel_render.h
index 2e7a372f83..51011091f7 100644
--- a/stb_voxel_render.h
+++ b/stb_voxel_render.h
@@ -3126,15 +3126,17 @@ static void stbvox_make_mesh_for_block_with_geo(stbvox_mesh_maker *mm, stbvox_po
       stbvox_mesh_vertex vmesh[6][4];
       stbvox_rotate rotate = { 0,0,0,0 };
       unsigned char simple_rot = rot;
-      int i;
+      int i, j;
       // we only need to do this for the displayed faces, but it's easier
       // to just do it up front; @OPTIMIZE check if it's faster to do it
       // for visible faces only
-      for (i=0; i < 6*4; ++i) {
-         int vert = stbvox_vertex_selector[0][i];
-         vert = stbvox_rotate_vertex[vert][rot];
-         vmesh[0][i] = stbvox_vmesh_pre_vheight[0][i]
-                     + stbvox_geometry_vheight[geo][vert];
+      for (i=0; i < 6; ++i) {
+         for (j=0; j < 4; ++j) {
+            int vert = stbvox_vertex_selector[i][j];
+            vert = stbvox_rotate_vertex[vert][rot];
+            vmesh[i][j] = stbvox_vmesh_pre_vheight[i][j]
+                        + stbvox_geometry_vheight[geo][vert];
+         }
       }
 
       basevert = stbvox_vertex_encode(pos.x, pos.y, pos.z << STBVOX_CONFIG_PRECISION_Z, 0,0);
@@ -3275,11 +3277,13 @@ static void stbvox_make_mesh_for_block_with_geo(stbvox_mesh_maker *mm, stbvox_po
 
       // build vertex mesh
       {
-         int i;
-         for (i=0; i < 6*4; ++i) {
-            int vert = stbvox_vertex_selector[0][i];
-            vmesh[0][i] = stbvox_vmesh_pre_vheight[0][i]
-                        + cube[vert];
+         int i, j;
+         for (i=0; i < 6; ++i) {
+            for (j=0; j < 4; ++j) {
+               int vert = stbvox_vertex_selector[i][j];
+               vmesh[i][j] = stbvox_vmesh_pre_vheight[i][j]
+                           + cube[vert];
+            }
          }
       }
 
@@ -3541,10 +3545,12 @@ int stbvox_get_buffer_size_per_quad(stbvox_mesh_maker *mm, int n)
 
 void stbvox_reset_buffers(stbvox_mesh_maker *mm)
 {
-   int i;
-   for (i=0; i < STBVOX_MAX_MESHES*STBVOX_MAX_MESH_SLOTS; ++i) {
-      mm->output_cur[0][i] = 0;
-      mm->output_buffer[0][i] = 0;
+   int i, j;
+   for (i=0; i < STBVOX_MAX_MESHES; ++i) {
+      for (j=0; j < STBVOX_MAX_MESH_SLOTS; ++j) {
+         mm->output_cur[i][j] = 0;
+         mm->output_buffer[i][j] = 0;
+      }
    }
 }
 
diff --git a/tests/caveview/cave_mesher.c b/tests/caveview/cave_mesher.c
index 1f76c89812..bbf79898b6 100644
--- a/tests/caveview/cave_mesher.c
+++ b/tests/caveview/cave_mesher.c
@@ -802,7 +802,7 @@ void remap_in_place(int bt, int rm)
 
 void mesh_init(void)
 {
-   int i;
+   int i, j;
 
    chunk_cache_mutex = SDL_CreateMutex();
    chunk_get_mutex   = SDL_CreateMutex();
@@ -814,17 +814,19 @@ void mesh_init(void)
    }
    //effective_blocktype[50] = 0; // delete torches
 
-   for (i=0; i < 6*256; ++i) {
-      if (minecraft_tex1_for_blocktype[0][i] == 40)
-         minecraft_color_for_blocktype[0][i] = 38 | 64; // apply to tex1
-      if (minecraft_tex1_for_blocktype[0][i] == 39)
-         minecraft_color_for_blocktype[0][i] = 39 | 64; // apply to tex1
-      if (minecraft_tex1_for_blocktype[0][i] == 105)
-         minecraft_color_for_blocktype[0][i] = 63; // emissive
-      if (minecraft_tex1_for_blocktype[0][i] == 212)
-         minecraft_color_for_blocktype[0][i] = 63; // emissive
-      if (minecraft_tex1_for_blocktype[0][i] == 80)
-         minecraft_color_for_blocktype[0][i] = 63; // emissive
+   for (i=0; i < 6; ++i) {
+      for (j=0; j < 256; ++j) {
+         if (minecraft_tex1_for_blocktype[i][j] == 40)
+            minecraft_color_for_blocktype[i][j] = 38 | 64; // apply to tex1
+         if (minecraft_tex1_for_blocktype[i][j] == 39)
+            minecraft_color_for_blocktype[i][j] = 39 | 64; // apply to tex1
+         if (minecraft_tex1_for_blocktype[i][j] == 105)
+            minecraft_color_for_blocktype[i][j] = 63; // emissive
+         if (minecraft_tex1_for_blocktype[i][j] == 212)
+            minecraft_color_for_blocktype[i][j] = 63; // emissive
+         if (minecraft_tex1_for_blocktype[i][j] == 80)
+            minecraft_color_for_blocktype[i][j] = 63; // emissive
+      }
    }
 
    for (i=0; i < 6; ++i) {
diff --git a/tests/resample_test.cpp b/tests/resample_test.cpp
index 21f874f18b..bb8ad82ef6 100644
--- a/tests/resample_test.cpp
+++ b/tests/resample_test.cpp
@@ -646,8 +646,9 @@ void verify_box(void)
 
 	resample_88(STBIR_FILTER_BOX);
 
-	for (i=0; i < sizeof(image88); ++i)
-		STBIR_ASSERT(image88[0][i] == output88[0][i]);
+	for (i=0; i < sizeof(image88) / sizeof(image88[0]); ++i)
+		for (j=0; j < sizeof(image88[0]); ++j)
+			STBIR_ASSERT(image88[i][j] == output88[i][j]);
 
 	t = 0;
 	for (j=0; j < 4; ++j)
@@ -685,12 +686,14 @@ void test_filters(void)
 
 	mtsrand(0);
 
-	for (i=0; i < sizeof(image88); ++i)
-		image88[0][i] = mtrand() & 255;
+	for (i=0; i < sizeof(image88) / sizeof(image88[0]); ++i)
+		for (j=0; j < sizeof(image88[0]); ++j)
+			image88[i][j] = mtrand() & 255;
 	verify_box();
 
-	for (i=0; i < sizeof(image88); ++i)
-		image88[0][i] = 0;
+	for (i=0; i < sizeof(image88) / sizeof(image88[0]); ++i)
+		for (j=0; j < sizeof(image88[0]); ++j)
+			image88[i][j] = 0;
 	image88[4][4] = 255;
 	verify_box();
 
