| Summary: | UBSan hits on a signed integer left-shift in KisFixedPoint | ||
|---|---|---|---|
| Product: | [Applications] krita | Reporter: | amyspark <amy> |
| Component: | General | Assignee: | amyspark <amy> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | dimula73 |
| Priority: | NOR | ||
| Version First Reported In: | git master (please specify the git hash!) | ||
| Target Milestone: | --- | ||
| Platform: | Compiled Sources | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/graphics/krita/commit/a7926219257fa30fbf1ebadcc5202846e302c472 | Version Fixed In: | |
| Sentry Crash Report: | |||
| Attachments: | Backtrace of breakpoint | ||
|
Description
amyspark
2022-03-25 19:23:49 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.
No, this is already UB, so the best way would be to explicitly override UBsan in these two codepaths with __attribute__((no_sanitize("undefined"))).
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 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? :)
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 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 A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1390 |