hb_graphite2_cluster_t::advance can apparently be negative #3128
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...as seen with HarfBuzz used by LibreOffice, with `instdir/program/soffice --headless --convert-to pdf` of doc/abi6073-2.doc from the LibreOffice crash- testing corpus when run under UBSan, > hb-graphite2.cc:361:15: runtime error: -1024 is outside the range of representable values of type 'unsigned int' > #0 in _hb_graphite2_shape at workdir/UnpackedTarball/harfbuzz/src/hb-graphite2.cc:361:15 > harfbuzz#1 in _hb_shape_plan_execute_internal(hb_shape_plan_t*, hb_font_t*, hb_buffer_t*, hb_feature_t const*, unsigned int) at workdir/UnpackedTarball/harfbuzz/src/./hb-shaper-list.hh:38:1 > harfbuzz#2 in hb_shape_plan_execute at workdir/UnpackedTarball/harfbuzz/src/hb-shape-plan.cc:453:14 > harfbuzz#3 in hb_shape_full at workdir/UnpackedTarball/harfbuzz/src/hb-shape.cc:139:19 > harfbuzz#4 in GenericSalLayout::LayoutText(ImplLayoutArgs&, SalLayoutGlyphsImpl const*) at vcl/source/gdi/CommonSalLayout.cxx:495:23 > harfbuzz#5 in OutputDevice::getFallbackLayout(LogicalFontInstance*, int, ImplLayoutArgs&, SalLayoutGlyphs const*) const at vcl/source/outdev/font.cxx:1232:21 > harfbuzz#6 in OutputDevice::ImplGlyphFallbackLayout(std::unique_ptr<SalLayout, std::default_delete<SalLayout> >, ImplLayoutArgs&, SalLayoutGlyphs const*) const at vcl/source/outdev/font.cxx:1300:48 > harfbuzz#7 in OutputDevice::ImplLayout(rtl::OUString const&, int, int, Point const&, long, long const*, SalLayoutFlags, vcl::TextLayoutCache const*, SalLayoutGlyphs const*) const at vcl/source/outdev/text.cxx:1332:22 > harfbuzz#8 in lcl_CreateLayout(SwTextGlyphsKey const&, __gnu_debug::_Safe_iterator<std::_Rb_tree_iterator<std::pair<SwTextGlyphsKey const, SwTextGlyphsData> >, std::__debug::map<SwTextGlyphsKey, SwTextGlyphsData, std::less<SwTextGlyphsKey>, std::allocator<std::pair<SwTextGlyphsKey const, SwTextGlyphsData> > >, std::bidirectional_iterator_tag>) at sw/source/core/txtnode/fntcache.cxx:233:33 > harfbuzz#9 in SwFntObj::GetCachedSalLayoutGlyphs(SwTextGlyphsKey const&) at sw/source/core/txtnode/fntcache.cxx:257:12 > harfbuzz#10 in SwFont::GetTextBreak(SwDrawTextInfo const&, long) at sw/source/core/txtnode/fntcache.cxx:2551:58 > harfbuzz#11 in SwTextSizeInfo::GetTextBreak(long, o3tl::strong_int<int, Tag_TextFrameIndex>, unsigned short, vcl::TextLayoutCache const*) const at sw/source/core/text/inftxt.cxx:450:20 > harfbuzz#12 in SwTextGuess::Guess(SwTextPortion const&, SwTextFormatInfo&, unsigned short) at sw/source/core/text/guess.cxx:205:26 > harfbuzz#13 in SwTextPortion::Format_(SwTextFormatInfo&) at sw/source/core/text/portxt.cxx:305:32 > harfbuzz#14 in SwTextPortion::Format(SwTextFormatInfo&) at sw/source/core/text/portxt.cxx:456:12 > harfbuzz#15 in SwLineLayout::Format(SwTextFormatInfo&) at sw/source/core/text/porlay.cxx:260:31 (where in frame harfbuzz#4 GenericSalLayout::LayoutText, pHbBuffer->props.direction is HB_DIRECTION_RTL, in case that is relevant). It is unclear to me whether it is sufficient to only change hb_graphite2_cluster_t::advance from signed to unsigned int, as there are other unsigned int variables (like curradv in _hb_graphite2_shape) whose value depend on hb_graphite2_cluster_t::advance, and which thus might also become negative. But unlike the float -> unsigned int conversion that UBSan warned about here (where gr_slot_origin_X() and xscale are float), those are signed int -> unsigned int conversions that do not cause undefined behavior. At least, with this change, the above --convert-to pdf and a full `make check screenshot` succeeded for me under without further UBSan warnings. (For the version of HarfBuzz optionally built as part of the LibreOffice build, this has been addressed with <https://git.libreoffice.org/core/+/6e53e03f752c2f85283c4d47efaaf0683299783c%5E!/> "external/harfbuzz: hb_graphite2_cluster_t::advance can apparently be negative.")
|
That wouldn't do much. cc @mhosken |
|
Or maybe it's enough for harfbuzz. But downstream clients might choke on negative advances. |
|
@mhosken any chance you can check this issue? |
|
I've discovered that our CoreText shaper can also generate negative advance. And nothing stops the OpenType shaper to do so either. So, merging this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...as seen with HarfBuzz used by LibreOffice, with
instdir/program/soffice --headless --convert-to pdfof doc/abi6073-2.doc from the LibreOffice crash-testing corpus when run under UBSan,(where in frame
#4GenericSalLayout::LayoutText, pHbBuffer->props.direction is HB_DIRECTION_RTL, in case that is relevant).It is unclear to me whether it is sufficient to only change hb_graphite2_cluster_t::advance from signed to unsigned int, as there are other
unsigned int variables (like curradv in _hb_graphite2_shape) whose value depend on hb_graphite2_cluster_t::advance, and which thus might also become negative.
But unlike the float -> unsigned int conversion that UBSan warned about here (where gr_slot_origin_X() and xscale are float), those are signed int -> unsigned int conversions that do not cause undefined behavior. At least, with this change, the above --convert-to pdf and a full
make check screenshotsucceeded for me under without further UBSan warnings.(For the version of HarfBuzz optionally built as part of the LibreOffice build, this has been addressed with https://git.libreoffice.org/core/+/6e53e03f752c2f85283c4d47efaaf0683299783c%5E!/ "external/harfbuzz: hb_graphite2_cluster_t::advance can apparently be
negative.")