Sanitize type 'size' parameters in our read/write file code
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Aug 21 2020, 7:13 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Commits

rB5ea1049e7520: Sanitize type 'size' parameters in our read/write file code

Summary

This patch tries to sanitize the types of our size parameters across our read and write code, which is currently fairly inconsistent (using int, uint, size_t...), by using size_t everywhere. Since in Blender file themselves we can only store chunk of size MAX_INT, added some asserts to ensure that as well.

See T79561: Sanitize size handling in write code for details.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D8672 (branched from master)

Build Status

Buildable 9850
Build 9850: arc lint + arc unit

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Aug 21 2020, 7:13 PM

Bastien Montagne (mont29) created this revision.

LGTM, only minor issues noted.

This patch adds some explicit casts which aren't needed, this is more a question of conventions though.

In the case of passing an int value, I'm not sure about doing this, it seems like there is no advantage, eg: writedata(wd, DNA1, (size_t)wd->sdna->data_len, wd->sdna->data);

Even in the cases where sizeof(..) * count is performed, the initial sizeof promotes the second number to a size_t, see: https://wiki.blender.org/wiki/Style_Guide/Best_Practice_C_Cpp
Although an argument could be made that relying on the order of operations in this case is hidden, we should probably agree on a convention since multiplying int's by sizeof is done in so many places.

source/blender/blenloader/intern/readfile.c
1206	Shouldn't an assert be added here for `size > UINT_MAX`?
source/blender/blenloader/intern/writefile.c
2357	This looks to be caused by clang-format not finding it's configuration

This revision now requires changes to proceed.Aug 22 2020, 9:45 AM

Update against latest master, address review comments.

Harbormaster completed remote builds in B9850: Diff 28242.Aug 27 2020, 5:12 PM

In D8672#211456, @Campbell Barton (campbellbarton) wrote:

This patch adds some explicit casts which aren't needed, this is more a question of conventions though.

In the case of passing an int value, I'm not sure about doing this, it seems like there is no advantage, eg: writedata(wd, DNA1, (size_t)wd->sdna->data_len, wd->sdna->data);

Even in the cases where sizeof(..) * count is performed, the initial sizeof promotes the second number to a size_t, see: https://wiki.blender.org/wiki/Style_Guide/Best_Practice_C_Cpp
Although an argument could be made that relying on the order of operations in this case is hidden, we should probably agree on a convention since multiplying int's by sizeof is done in so many places.

I personally tend to prefer to be explicit about actual type in those cases, even if it makes code a bit more verbose. QTCreator is also more happy (it generates warnings otherwise). And I also see it as a good incentive to try to reduce this mixing of int´s with size_t´s and the like...

But if other devs consider those casts as too annoying, I don´t care enough about those to fight to keep them either.

source/blender/blenloader/intern/readfile.c
1206	Indeed.

Any updates here? wouldn't mind adding this to master soon…

I think in general it's best to cast as little as possible, since it means you can change e.g. the return value of a function from or a member variable an int to a pointer, and wrong code will be compiled without error.

In this case it seems fine.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2020, 6:50 PM

Closed by commit rB5ea1049e7520: Sanitize type 'size' parameters in our read/write file code (authored by Bastien Montagne (mont29)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Bastien Montagne (mont29) added a commit: rB5ea1049e7520: Sanitize type 'size' parameters in our read/write file code.