Bug 451903 - UBSan hits on a signed integer left-shift in KisFixedPoint
Summary: UBSan hits on a signed integer left-shift in KisFixedPoint
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: General (other bugs)
Version First Reported In: git master (please specify the git hash!)
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: amyspark
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-25 19:23 UTC by amyspark
Modified: 2022-05-17 11:34 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Backtrace of breakpoint (2.07 KB, text/plain)
2022-03-25 19:23 UTC, amyspark
Details

Note You need to log in before you can comment on or make changes to this bug.
Description amyspark 2022-03-25 19:23:49 UTC
Created attachment 147730 [details]
Backtrace of breakpoint

SUMMARY
***
NOTE: If you are reporting a crash, please try to attach a backtrace with debug symbols.
See https://community.kde.org/Guidelines_and_HOWTOs/Debugging/How_to_create_useful_crash_reports
***

Running Krita with UBSan on the xsimd preparation branch yields a lot of UBSan errors. The only one I could not fix is:

/home/amalia/krita/src/libs/image/kis_fixed_point_maths.h:22:20: runtime error: left shift of negative value -2
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/amalia/krita/src/libs/image/kis_fixed_point_maths.h:22:20 in 

STEPS TO REPRODUCE
1.  Build Krita with `ECM_ENABLE_SANITIZER` set in CMake to `undefined`.
2.  Run Krita and start painting.

OBSERVED RESULT
None, just the UBSan report.

EXPECTED RESULT
None.

SOFTWARE/OS VERSIONS
Windows: 
macOS: 
Linux/KDE Plasma: 
(available in About System)
KDE Plasma Version: 
KDE Frameworks Version: 
Qt Version: 

ADDITIONAL INFORMATION
d47a33aa9c
Comment 1 Dmitry Kazakov 2022-03-28 15:55:34 UTC
Hi, amyspark!

There is a comment about that in KisFixedPoint telling exctly about that:

    KisFixedPoint& operator*=(const KisFixedPoint& x) {
        /**
         * Until C++20 `d >>= 8` is "implementation defined" for negative `d`.
         * But we have a unittest that confirms that the our compiler handles
         * that in an expected way
         */

        d *= x.d;
        d >>= 8;
        return *this;
    }

Since C++20 this code will become valid as it is, and it seems to be valid at least on AMD64 in the compilers we use. Do you have an idea if we can rewrite that in C++14 or C++17 without sacrificing performance? This code is used in the hottest path in the transform tool, so doing extra work is not desirable.
Comment 2 amyspark 2022-03-28 20:34:22 UTC
No, this is already UB, so the best way would be to explicitly override UBsan in these two codepaths with __attribute__((no_sanitize("undefined"))).
Comment 3 Dmitry Kazakov 2022-03-29 10:59:17 UTC
Git commit e3d359608b9d6038b2428e8a49b833e7a5188ba4 by Dmitry Kazakov, on behalf of L. E. Segovia.
Committed on 29/03/2022 at 10:55.
Pushed by dkazakov into branch 'master'.

Fix Clang+UBSan build

This addresses a lot of build and runtime errors flagged by Clang 13
with the UBSan sanitizer enabled.

M  +0    -2    libs/brush/kis_brush.cpp
M  +3    -1    libs/image/brushengine/KisPaintOpPresetUpdateProxy.h
M  +1    -1    libs/image/brushengine/kis_locked_properties_proxy.h
M  +2    -1    libs/image/kis_gradient_shape_strategy.h
M  +1    -2    libs/image/kis_update_job_item.h
M  +1    -1    libs/ui/KisReferenceImagesDecoration.h
M  +1    -1    libs/ui/canvas/kis_update_info.h
M  +1    -1    libs/ui/widgets/kis_paintop_presets_editor.cpp
M  +1    -1    libs/ui/widgets/kis_paintop_presets_editor.h
M  +1    -1    libs/widgetutils/xmlgui/kactioncollection.cpp

https://invent.kde.org/graphics/krita/commit/e3d359608b9d6038b2428e8a49b833e7a5188ba4
Comment 4 Dmitry Kazakov 2022-03-29 11:01:13 UTC
Hi, amyspark!

> No, this is already UB, so the best way would be to explicitly override UBsan in these two codepaths with __attribute__((no_sanitize("undefined")))

Can you make a patch for it? I don't really know where to put this attribute. Into the function header? Or somewhere else? :)
Comment 5 amyspark 2022-04-03 13:30:22 UTC
Git commit bcff23b8f01e483fdde3807cb19eef6ad7ea4eca by L. E. Segovia.
Committed on 03/04/2022 at 13:29.
Pushed by lsegovia into branch 'master'.

Workaround crash on null action

Technically, the action *is* null. And we already are handling raw
gunpowder, as UBSan says...
Related: bug 452063

M  +1    -1    libs/widgetutils/xmlgui/kactioncollection.cpp

https://invent.kde.org/graphics/krita/commit/bcff23b8f01e483fdde3807cb19eef6ad7ea4eca
Comment 6 amyspark 2022-05-04 15:50:32 UTC
Git commit a7926219257fa30fbf1ebadcc5202846e302c472 by L. E. Segovia.
Committed on 04/05/2022 at 14:24.
Pushed by lsegovia into branch 'master'.

Fix UBSan warning on KisFixedPoint

M  +13   -11   libs/image/kis_fixed_point_maths.h

https://invent.kde.org/graphics/krita/commit/a7926219257fa30fbf1ebadcc5202846e302c472
Comment 7 Bug Janitor Service 2022-05-17 11:34:30 UTC
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1390