Replacing from a fixed -sized to just the size needed
Details
Details
- Reviewers
Ray Molenkamp (LazyDodo) Sybren A. Stüvel (sybren) Kévin Dietrich (kevindietrich) - Group Reviewers
Alembic - Maniphest Tasks
- T51321: AlembicObjectPath has a fixed-size char[]
Diff Detail
Diff Detail
Event Timeline
| 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. | |
Comment Actions
@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.
Comment Actions
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
Comment Actions
- look up BLI_strdup it'll be easier than mucking about with malloc and strncpy
- 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.
Comment Actions
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. | |
Comment Actions
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
Comment Actions
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)
Comment Actions
I agree with @Ray Molenkamp (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.