Fix T45471: Blend file: Bad old_addr handling in mesh's customdata writing.

Issue is rather well explained in T45471: our current customdata writing code easily generates several different blocks in blend file with same 'old' address. This is bad, because those addresses are used as 'uid' during reading process (it kind of work in Blender's own reading process, by mere luck mostly, but breaks the file specs).

Solution (suggested by Campbell, thanks) implemented by this patch is to avoid duplicating everything, and instead just overwrite what we needs to skip some cdlayers on write:
* the CustomData's `totlayer` number;
* the CustomData's `layers` array of CustomDataLayer (keeping its original address using the `writestruct_at_address` helper).

New design allows us to get completely rid of the no_free flag stuff in `write_customdata()`.

Note that this implies written data is **not** directly valid from Blend PoV, since its written typemap does not match written layers (this is not an issue because typemap is rebuilt on read anyway - and it's easy to fix this if really needed).

Also, the backward compatibility saving of mface data remains an issue here, see comment in code.

Reviewers: sergey, campbellbarton

Projects: #bf_blender

Maniphest Tasks: T45471

Differential Revision: https://developer.blender.org/D1425

1 files changed, 66 insertions, 58 deletions

diff --git a/source/blender/blenloader/intern/writefile.c b/source/blender/blenloader/intern/writefile.c
index e03db11e71b..f5d35e45fc0 100644
--- a/source/blender/blenloader/intern/writefile.c
+++ b/source/blender/blenloader/intern/writefile.c
@@ -1906,37 +1906,20 @@ static void write_grid_paint_mask(WriteData *wd, int count, GridPaintMask *grid_
 	}
 }
 
-static void write_customdata(WriteData *wd, ID *id, int count, CustomData *data, int partial_type, int partial_count)
+static void write_customdata(
+        WriteData *wd, ID *id, int count, CustomData *data, CustomDataLayer *layers,
+        int partial_type, int partial_count)
 {
 	int i;
 
-	int nofree_buff[128];
-	int *nofree;
-
 	/* write external customdata (not for undo) */
 	if (data->external && !wd->current)
 		CustomData_external_write(data, id, CD_MASK_MESH, count, 0);
 
-	if (data->totlayer > ARRAY_SIZE(nofree_buff)) {
-		nofree = MEM_mallocN(sizeof(*nofree) * (size_t)data->totlayer, __func__);
-	}
-	else {
-		nofree = nofree_buff;
-	}
-
-	for (i = 0; i < data->totlayer; i++) {
-		nofree[i] = (data->layers[i].flag & CD_FLAG_NOFREE);
-		data->layers[i].flag &= ~CD_FLAG_NOFREE;
-	}
-
-	writestruct(wd, DATA, "CustomDataLayer", data->maxlayer, data->layers);
+	writestruct_at_address(wd, DATA, "CustomDataLayer", data->totlayer, data->layers, layers);
  
 	for (i = 0; i < data->totlayer; i++) {
-		data->layers[i].flag |= nofree[i];
-	}
-
-	for (i = 0; i < data->totlayer; i++) {
-		CustomDataLayer *layer= &data->layers[i];
+		CustomDataLayer *layer = &layers[i];
 		const char *structname;
 		int structnum, datasize;
 
@@ -1974,10 +1957,6 @@ static void write_customdata(WriteData *wd, ID *id, int count, CustomData *data,
 
 	if (data->external)
 		writestruct(wd, DATA, "CustomDataExternal", 1, data->external);
-
-	if (nofree != nofree_buff) {
-		MEM_freeN(nofree);
-	}
 }
 
 static void write_meshes(WriteData *wd, ListBase *idbase)
@@ -1991,6 +1970,12 @@ static void write_meshes(WriteData *wd, ListBase *idbase)
 
 	mesh= idbase->first;
 	while (mesh) {
+		CustomDataLayer *vlayers = NULL, vlayers_buff[CD_TEMP_CHUNK_SIZE];
+		CustomDataLayer *elayers = NULL, elayers_buff[CD_TEMP_CHUNK_SIZE];
+		CustomDataLayer *flayers = NULL, flayers_buff[CD_TEMP_CHUNK_SIZE];
+		CustomDataLayer *llayers = NULL, llayers_buff[CD_TEMP_CHUNK_SIZE];
+		CustomDataLayer *players = NULL, players_buff[CD_TEMP_CHUNK_SIZE];
+
 		if (mesh->id.us>0 || wd->current) {
 			/* write LibData */
 			if (!save_for_old_blender) {
@@ -2007,17 +1992,22 @@ static void write_meshes(WriteData *wd, ListBase *idbase)
 				memset(&mesh->fdata, 0, sizeof(mesh->fdata));
 #endif /* USE_BMESH_SAVE_WITHOUT_MFACE */
 
-				/* Bummer! We need to do the copy *before* writing mesh's struct itself,
-				 * because we eliminate NO_COPY & TEMPORARY layers here, which means
-				 * **number of layers (data.totlayer) may be smaller!**
-				 * If we do not do that, we can get crash by buffer-overflow on reading, see T44461. */
-				CustomData_copy(&old_mesh->vdata, &mesh->vdata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totvert);
-				CustomData_copy(&old_mesh->edata, &mesh->edata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totedge);
+				/**
+				 * Those calls:
+				 *   - Reduce mesh->xdata.totlayer to number of layers to write.
+				 *   - Fill xlayers with those layers to be written.
+				 * Note that mesh->xdata is from now on invalid for Blender, but this is why the whole mesh is
+				 * a temp local copy!
+				 */
+				CustomData_file_write_prepare(&mesh->vdata, &vlayers, vlayers_buff, ARRAY_SIZE(vlayers_buff));
+				CustomData_file_write_prepare(&mesh->edata, &elayers, elayers_buff, ARRAY_SIZE(elayers_buff));
 #ifndef USE_BMESH_SAVE_WITHOUT_MFACE  /* Do not copy org fdata in this case!!! */
-				CustomData_copy(&old_mesh->fdata, &mesh->fdata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totface);
+				CustomData_file_write_prepare(&mesh->fdata, &flayers, flayers_buff, ARRAY_SIZE(flayers_buff));
+#else
+				flayers = flayers_buff;
 #endif
-				CustomData_copy(&old_mesh->ldata, &mesh->ldata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totloop);
-				CustomData_copy(&old_mesh->pdata, &mesh->pdata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totpoly);
+				CustomData_file_write_prepare(&mesh->ldata, &llayers, llayers_buff, ARRAY_SIZE(llayers_buff));
+				CustomData_file_write_prepare(&mesh->pdata, &players, players_buff, ARRAY_SIZE(players_buff));
 
 				writestruct_at_address(wd, ID_ME, "Mesh", 1, old_mesh, mesh);
 
@@ -2028,20 +2018,12 @@ static void write_meshes(WriteData *wd, ListBase *idbase)
 				writedata(wd, DATA, sizeof(void *)*mesh->totcol, mesh->mat);
 				writedata(wd, DATA, sizeof(MSelect) * mesh->totselect, mesh->mselect);
 
-				write_customdata(wd, &mesh->id, mesh->totvert, &mesh->vdata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totedge, &mesh->edata, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totvert, &mesh->vdata, vlayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totedge, &mesh->edata, elayers, -1, 0);
 				/* fdata is really a dummy - written so slots align */
-				write_customdata(wd, &mesh->id, mesh->totface, &mesh->fdata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totloop, &mesh->ldata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totpoly, &mesh->pdata, -1, 0);
-
-				CustomData_free(&mesh->vdata, mesh->totvert);
-				CustomData_free(&mesh->edata, mesh->totedge);
-#ifndef USE_BMESH_SAVE_WITHOUT_MFACE
-				CustomData_free(&mesh->fdata, mesh->totface);
-#endif
-				CustomData_free(&mesh->ldata, mesh->totloop);
-				CustomData_free(&mesh->pdata, mesh->totpoly);
+				write_customdata(wd, &mesh->id, mesh->totface, &mesh->fdata, flayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totloop, &mesh->ldata, llayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totpoly, &mesh->pdata, players, -1, 0);
 
 				/* restore pointer */
 				mesh = old_mesh;
@@ -2066,14 +2048,24 @@ static void write_meshes(WriteData *wd, ListBase *idbase)
 				mesh->edit_btmesh = NULL;
 
 				/* now fill in polys to mfaces */
+				/* XXX This breaks writing desing, by using temp allocated memory, which will likely generate
+				 *     doublons in stored 'old' addresses.
+				 *     This is very bad, but do not see easy way to avoid this, aside from generating those data
+				 *     outside of save process itself.
+				 *     Maybe we can live with this, though?
+				 */
 				mesh->totface = BKE_mesh_mpoly_to_mface(&mesh->fdata, &old_mesh->ldata, &old_mesh->pdata,
 				                                        mesh->totface, old_mesh->totloop, old_mesh->totpoly);
 
 				BKE_mesh_update_customdata_pointers(mesh, false);
 
-				/* See comment above. Note that loop/poly data are ignored here, and face ones are already handled. */
-				CustomData_copy(&old_mesh->vdata, &mesh->vdata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totvert);
-				CustomData_copy(&old_mesh->edata, &mesh->edata, CD_MASK_EVERYTHING, CD_REFERENCE, mesh->totedge);
+				CustomData_file_write_prepare(&mesh->vdata, &vlayers, vlayers_buff, ARRAY_SIZE(vlayers_buff));
+				CustomData_file_write_prepare(&mesh->edata, &elayers, elayers_buff, ARRAY_SIZE(elayers_buff));
+				CustomData_file_write_prepare(&mesh->fdata, &flayers, flayers_buff, ARRAY_SIZE(flayers_buff));
+#if 0
+				CustomData_file_write_prepare(&mesh->ldata, &llayers, llayers_buff, ARRAY_SIZE(llayers_buff));
+				CustomData_file_write_prepare(&mesh->pdata, &players, players_buff, ARRAY_SIZE(players_buff));
+#endif
 
 				writestruct_at_address(wd, ID_ME, "Mesh", 1, old_mesh, mesh);
 
@@ -2084,24 +2076,40 @@ static void write_meshes(WriteData *wd, ListBase *idbase)
 				writedata(wd, DATA, sizeof(void *)*mesh->totcol, mesh->mat);
 				/* writedata(wd, DATA, sizeof(MSelect) * mesh->totselect, mesh->mselect); */ /* pre-bmesh NULL's */
 
-				write_customdata(wd, &mesh->id, mesh->totvert, &mesh->vdata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totedge, &mesh->edata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totface, &mesh->fdata, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totvert, &mesh->vdata, vlayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totedge, &mesh->edata, elayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totface, &mesh->fdata, flayers, -1, 0);
 				/* harmless for older blender versioins but _not_ writing these keeps file size down */
 #if 0
-				write_customdata(wd, &mesh->id, mesh->totloop, &mesh->ldata, -1, 0);
-				write_customdata(wd, &mesh->id, mesh->totpoly, &mesh->pdata, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totloop, &mesh->ldata, llayers, -1, 0);
+				write_customdata(wd, &mesh->id, mesh->totpoly, &mesh->pdata, players, -1, 0);
 #endif
 
-				CustomData_free(&mesh->vdata, mesh->totvert);
-				CustomData_free(&mesh->edata, mesh->totedge);
 				CustomData_free(&mesh->fdata, mesh->totface);
+				flayers = NULL;
 
 				/* restore pointer */
 				mesh = old_mesh;
 #endif /* USE_BMESH_SAVE_AS_COMPAT */
 			}
 		}
+
+		if (vlayers && vlayers != vlayers_buff) {
+			MEM_freeN(vlayers);
+		}
+		if (elayers && elayers != elayers_buff) {
+			MEM_freeN(elayers);
+		}
+		if (flayers && flayers != flayers_buff) {
+			MEM_freeN(flayers);
+		}
+		if (llayers && llayers != llayers_buff) {
+			MEM_freeN(llayers);
+		}
+		if (players && players != players_buff) {
+			MEM_freeN(players);
+		}
+
 		mesh= mesh->id.next;
 	}
 }