Bug 225335 – Add bounds checks around calls to GlyphBuffer::stringOffsetAt()

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Add bounds checks around calls to GlyphBuffer::stringOffsetAt()

[Myles C. Maxfield]

Created attachment 427622 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/75663608>

[Myles C. Maxfield]

Created attachment 427624 [details]
Patch

[Geoffrey Garen]

r=me too

[Geoffrey Garen]

Comment on attachment 427624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427624&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:398
>          auto stringOffset = glyphBuffer.stringOffsetAt(i);

Given the observation that stringOffsetAt may return an invalid value, indicating that CoreText did not find the string offset we wanted:

(1) Can we just check for kCFNotFound (i.e., -1); that would be a lot more direct and clear. Do we benefit from this more abstract comparison here, and do we expect CoreText to return whacky values other than kCFNotFound for some reason?

(2) Can stringOffsetAt return an Optional<GlyphBufferStringOffset> in order to become self-documenting?

I don't think these questions block landing; but would be good to address.

[Myles C. Maxfield]

Comment on attachment 427624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427624&action=review

>> Source/WebCore/platform/graphics/WidthIterator.cpp:398
>>          auto stringOffset = glyphBuffer.stringOffsetAt(i);
> 
> Given the observation that stringOffsetAt may return an invalid value, indicating that CoreText did not find the string offset we wanted:
> 
> (1) Can we just check for kCFNotFound (i.e., -1); that would be a lot more direct and clear. Do we benefit from this more abstract comparison here, and do we expect CoreText to return whacky values other than kCFNotFound for some reason?
> 
> (2) Can stringOffsetAt return an Optional<GlyphBufferStringOffset> in order to become self-documenting?
> 
> I don't think these questions block landing; but would be good to address.

(1) Given that these values can come from another framework, I think we should be defensive.

(2) It can! Good idea.

[Myles C. Maxfield]

Created attachment 428094 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 428095 [details]
Patch for committing

[EWS]

Committed r277232 (237501@main): <https://commits.webkit.org/237501@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428095 [details].

[Lauro Moura]

Comment on attachment 428095 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=428095&action=review

> Source/WebCore/platform/graphics/GlyphBuffer.h:81
> +        if (result < 0 || static_cast<unsigned>(result) >= stringLength)

This is raising -Wtype-limits warnings on non-CG platforms due to GlyphBufferStringOffset being 'unsigned' there. What would be the proper way to silence them? Maybe move the result<0 comparison to a USE(CG) block?

Example:

In file included from WebCore/PrivateHeaders/WebCore/Font.h:30,                                                                                                                                                    
                 from WebCore/PrivateHeaders/WebCore/FontCascade.h:28,                                                                                                                                             
                 from WebCore/PrivateHeaders/WebCore/GraphicsContext.h:32,                                                                                                                                         
                 from WebCore/PrivateHeaders/WebCore/FrameView.h:31,                                                                                                                                               
                 from ../../Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:56:                                                                                                                 
WebCore/PrivateHeaders/WebCore/GlyphBuffer.h: In member function ‘WTF::Optional<unsigned int> WebCore::GlyphBuffer::checkedStringOffsetAt(unsigned int, unsigned int) const’:                                      
WebCore/PrivateHeaders/WebCore/GlyphBuffer.h:81:20: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]                                                                            
   81 |         if (result < 0 || static_cast<unsigned>(result) >= stringLength)                                                                                                                                   
      |