Page MenuHome

Fix T51321 - AlembicObjectPath has a fixed-size char[]
Needs RevisionPublic

Authored by João Seixas (j_seixas) on Dec 8 2017, 6:48 PM.

Details

Summary

Replacing from a fixed -sized to just the size needed

Diff Detail

Event Timeline

João Seixas (j_seixas) retitled this revision from T51321 - AlembicObjectPath has a fixed-size char[] to T51321.Dec 8 2017, 7:00 PM
LazyDodo (LazyDodo) requested changes to this revision.Dec 8 2017, 7:00 PM

The memory allocated is never freed, which is a problem as well.

This revision now requires changes to proceed.Dec 8 2017, 7:00 PM
source/blender/alembic/intern/alembic_capi.cc
178

this will allocate the size of a pointer, also it's preferred to use the guarded memory allocator (MEM_* functions) like the rest of the file does.

579

same.

source/blender/alembic/intern/alembic_capi.cc
179

since since abc_path->path is now a pointer as well, this will copy at most 4/8 bytes and lack a zero teminator.

@João Seixas (j_seixas) thansk for your patch.

But please use more descriptive task names. For example instead of "T51321", something like: "Fix T51321 - AlembicObjectPath has a fixed-size char[]. (well it seems you did that and then reverted back to the short version, why? :)
This helps triaging the patches much faster. Also, don't add all projects to reviewer. For instancing, "Staging" and "2.8" are not related to this at all.

Aside from that a patch must have a description. This is ultimately the commit message that would be committed. You can even keep it updated with the planned commit message, followed by some message specific aimed at reviewers that can be suppressed in the final patch.

João Seixas (j_seixas) edited the summary of this revision. (Show Details)

I tried to look for the MEM_* functions, but i need some help in there. Also, i don't think the free() is 100% correct over there

João Seixas (j_seixas) retitled this revision from T51321 to Fix T51321 - AlembicObjectPath has a fixed-size char[].Dec 8 2017, 10:25 PM
LazyDodo (LazyDodo) added a comment.EditedDec 8 2017, 10:35 PM
  1. look up BLI_strdup it'll be easier than mucking about with malloc and strncpy
  2. that free is 100% wrong there, you'd be passing on a pointer to freed memory, very bad! find out what structure this is being written into and where it is freed, then add the additional logic to free this field there.
Kévin Dietrich (kevindietrich) requested changes to this revision.Dec 8 2017, 10:43 PM

I don't recall whether or not the paths are written to the blender file upon saving, if yes this might need a bit of rework. At the moment I don't have a build environment for Blender set up, so I can't try the patch, or scrutinize it more deeply. I made a few comments though.

source/blender/alembic/intern/alembic_capi.cc
178

Should something like:

abc_path->path = static_cast<char *>(MEM_mallocN(sizeof(char) * full_name.size()), "Alembic object path str");

Also, I prefer to be consistent with the code style, and keep using C++ style casting.

179

Should something like:

BLI_strncpy(abc_path->path, full_name.c_str(), full_name.size());
182

This will free the path directly and destroy its content, but we need it for the UI. Freeing of the strings should be done in cachefile.c before the line BLI_freelistN(&cache_file->object_paths);.

581–582

Should be something like:

abc_path->path = static_cast<char *>(MEM_mallocN(sizeof(char) * full_name.size()), "Alembic object path str");
BLI_strncpy(abc_path->path, full_name.c_str(), full_name.size());
585

Here as well, we shouldn't free directly.

This revision now requires changes to proceed.Dec 8 2017, 10:43 PM
João Seixas (j_seixas) updated this revision to Diff 9674.EditedDec 9 2017, 12:02 AM

I think i corrected everything. Also i added + 1 in the string size for making sure the zero terminator was passed.
In the free() i tried to use de MEM_freeN() but it was giving me some compilation errors

i still think that

abs_path->path = BLI_strdup(full_name.c_str());

is nicer than

abc_path->path = static_cast<char *>(MEM_mallocN(sizeof(char) * full_name.size()), "Alembic object path str");
BLI_strncpy(abc_path->path, full_name.c_str(), full_name.size());

but ultimately it's up to @Kévin Dietrich (kevindietrich) and @Sybren A. Stüvel (sybren)

Sybren A. Stüvel (sybren) requested changes to this revision.Dec 9 2017, 12:12 PM

I agree with @LazyDodo (LazyDodo) on using BLI_strdup. The rest of the patch looks good, but I'd have to actually try it out with ASAN enabled to check it properly.

This revision now requires changes to proceed.Dec 9 2017, 12:12 PM