Harden checks in datatoc_icon binary
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Dec 2 2020, 10:34 AM.

Details

Reviewers

Brecht Van Lommel (brecht)
Dalai Felinto (dfelinto)
Bastien Montagne (mont29)

Commits

rBe81034f75d09: Harden checks in datatoc_icon binary

Summary

The goal of the change is to perform check for attempts of icons
being overwritten on canvas. The check is based on checking original
coordinate of icons against all read icons. If there are two icon
files which have same original an error will be reported. The report
includes both file names to make it easier to troubleshoot.

This change will allow to early-on catch issues which we currently
have with the release environment: official Linux builds might have
different icon from Blender compiled locally. This is because the
order in which directory listing is traversed is not defined, so
it's like a race condition between two files to win the place in
the final canvas.

There is still possible improvement in the code to move more fields
into the context structure. This is beyond of goal of this change.

Note that before committing this change icons must be brought back
to their consistent state. Otherwise the build will fail.

Diff Detail

Repository: rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Dec 2 2020, 10:34 AM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B11512: Diff 31525.Dec 2 2020, 10:37 AM

make icons is failing for me at the moment on Linux.

SUMMARY: AddressSanitizer: 2515865 byte(s) leaked in 1930 allocation(s).
Traceback (most recent call last):
  File "/home/dfelinto/src/blender/blender/release/datafiles/blender_icons_update.py", line 67, in <module>
    run(cmd)
  File "/home/dfelinto/src/blender/blender/release/datafiles/blender_icons_update.py", line 11, in run
    subprocess.check_call(cmd)
  File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '('/home/dfelinto/src/blender/release/master/bin/blender', '--background', '--factory-startup', '-noaudio', '--python', '/home/dfelinto/src/blender/blender/release/datafiles/../../source/blender/datatoc/datatoc_icon_split.py', '--', '--image=/home/dfelinto/src/blender/blender/release/datafiles/blender_icons16.png', '--output=/home/dfelinto/src/blender/blender/release/datafiles/blender_icons16', '--output_prefix=icon16_', '--name_style=UI_ICONS', '--parts_x', '26', '--parts_y', '30', '--minx', '3', '--maxx', '53', '--miny', '3', '--maxy', '8', '--minx_icon', '2', '--maxx_icon', '2', '--miny_icon', '2', '--maxy_icon', '2', '--spacex_icon', '1', '--spacey_icon', '1')' returned non-zero exit status 1.
make: *** [GNUmakefile:494: icons] Error 1

That makes testing this hard for me.

@Dalai Felinto (dfelinto), Doesn't really seem to be related to the change, or to the icons build process. Is the confusion of ASAN/LSAN reporting non-zero code because there are detected error, and the make icons can't really if its Blender crash or other type of error happenned.

Suggestion: try non-ASAN build, or set the entironment variable LSAN_OPTIONS to exitcode=0: LSAN_OPTIONS=exitcode=0 make icons.

@Sergey Sharybin (sergey) I tried the following:

Remove //release/datafiles/blender_icons16
Remove //release/datafiles/blender_icons32
make icons (bypassing ASAN, thanks).
git add everything minus the submodules

git status
	deleted:    release/datafiles/blender_icons16/icon16_brush_blob.dat
	deleted:    release/datafiles/blender_icons16/icon16_clip.dat
	modified:   release/datafiles/blender_icons16/icon16_force_boid.dat
	deleted:    release/datafiles/blender_icons16/icon16_fund.dat
	deleted:    release/datafiles/blender_icons16/icon16_sculpt_dyntopo.dat
	deleted:    release/datafiles/blender_icons16/icon16_sealed.dat
	modified:   release/datafiles/blender_icons32/icon32_axis_front.dat
	deleted:    release/datafiles/blender_icons32/icon32_brush_blob.dat
	deleted:    release/datafiles/blender_icons32/icon32_clip.dat
	modified:   release/datafiles/blender_icons32/icon32_force_boid.dat
	deleted:    release/datafiles/blender_icons32/icon32_fund.dat
	modified:   release/datafiles/blender_icons32/icon32_mod_offset.dat
	deleted:    release/datafiles/blender_icons32/icon32_sculpt_dyntopo.dat
	deleted:    release/datafiles/blender_icons32/icon32_sealed.dat
	modified:   release/datafiles/prvicons.png

Then I get the following error:
make[3]: *** No rule to make target '//release/datafiles/blender_icons16/icon16_clip.dat', needed by 'release/datafiles/blender_icons16.png'. Stop.

I guess the patch is only exposing the problem, not causing it. But that means there are icons that no longer can be re-created at the moment, and we need this fixed before we can proceed with the change from this patch.

Code looks fine.

This revision is now accepted and ready to land.Dec 2 2020, 8:13 PM

@Dalai Felinto (dfelinto), Think it would mean that list of icons in source/blender/editors/datafiles/CMakeLists.txt (ICON_NAMES) got out of sync from source/blender/editors/include/UI_icons.h.

There seems to be no automatic script to neither re-generate the list based on UI_icons.h nor script to validate things are in consistent state. So someone needs to do manual work, it seems :(
Or re-write the CMake rules in a way that UI_icons.h is the only source of truth.