Issue 7127056: If getAdvance fails, getAdvanceData should not assert, but ignored (Closed)
|
Created:
11 years, 7 months ago by edisonn Modified:
11 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionIf getAdvance fails, getAdvanceData should not assert, but ignored.
Committed: https://code.google.com/p/skia/source/detail?r=7341
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #MessagesTotal messages: 27
Why is getAdvance failing? It shouldn't.
Sign in to reply to this message.
When we render skp's if the input skp has issues, we should not assert on user input, the skp. We should report the error, and let the caller signal however he likes. Mike also suggested to fill the data with 0's, but I would rather report NULL ... I will see what Mike/reviewers say about that. On Fri, Jan 18, 2013 at 1:57 PM, <vandebo@chromium.org> wrote: > Why is getAdvance failing? It shouldn't. > > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >
Sign in to reply to this message.
On 2013/01/18 19:03:40, edisonn wrote: > When we render skp's if the input skp has issues, we should not assert on > user input, the skp. We should report the error, and let the caller signal > however he likes. So this is another case where the local fonts don't match the fonts used when the .skp was generated. Any text is the result is almost guaranteed to be corrupt. I would argue that the assert is valid and shouldn't be removed.
Sign in to reply to this message.
I do not agree. If I generate a bad SKP for any reason, we should not assert but let the user the skp is not good. On Fri, Jan 18, 2013 at 2:10 PM, <vandebo@chromium.org> wrote: > On 2013/01/18 19:03:40, edisonn wrote: > >> When we render skp's if the input skp has issues, we should not assert >> > on > >> user input, the skp. We should report the error, and let the caller >> > signal > >> however he likes. >> > > So this is another case where the local fonts don't match the fonts used > when the .skp was generated. Any text is the result is almost > guaranteed to be corrupt. > > I would argue that the assert is valid and shouldn't be removed. > > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >
Sign in to reply to this message.
On 2013/01/18 19:14:05, edisonn wrote: > I do not agree. If I generate a bad SKP for any reason, we should not > assert but let the user the skp is not good. There's a cost to handling errors. It requires more code and more code makes things harder to change and maintain. This error *should* only occur when you're trying to play back a .skp in an inappropriate environment - but it's still important to check that it doesn't happen at any other time. It would make sense to spit out a log message suggesting what might be wrong in the .skp case, sure, but the code should still stop there. To properly handle this case, I think you'd want to add some kind of error code to getAdvanceTypefaceMetrics and plumb that through the consumers and all the way to the top level. So during .skp playback it could see, oh, the fonts are wrong and report that to the user. But that's a lot of effort for a problem that doesn't happen in production. > On Fri, Jan 18, 2013 at 2:10 PM, <mailto:vandebo@chromium.org> wrote: > > > On 2013/01/18 19:03:40, edisonn wrote: > > > >> When we render skp's if the input skp has issues, we should not assert > >> > > on > > > >> user input, the skp. We should report the error, and let the caller > >> > > signal > > > >> however he likes. > >> > > > > So this is another case where the local fonts don't match the fonts used > > when the .skp was generated. Any text is the result is almost > > guaranteed to be corrupt. > > > > I would argue that the assert is valid and shouldn't be removed. > > > > > https://codereview.appspot.**com/7127056/%3Chttps://codereview.appspot.com/71...> > >
Sign in to reply to this message.
A simpler fix would probably be to write the OS type in the .skp file and not do PDF text drawing if the OS doesn't match.
Sign in to reply to this message.
Mike/Ben, how do you prefer to fix this? On Fri, Jan 18, 2013 at 2:28 PM, <vandebo@chromium.org> wrote: > would probably be to write the OS type in the .skp file > and not do PDF text drawing if the OS doesn' >
Sign in to reply to this message.
If we say drawText, and I pass a bad glyphID, I do not expect it to assert. I'm OK if we draw whatever (garbarge, a box, empty), but we should not crash in debug builds. getAdvancedTypefaceMetrics seems analogous to me. If we want to return an error code, that might be fine, but I don't think this should assert.
Sign in to reply to this message.
Steve sked for the callstack #0 0x0000eb42 in SkAdvancedTypefaceMetrics::AdvanceMetric<short>* skia_advanced_typeface_metrics_utils::getAdvanceData<short, __CTFont const*>(__CTFont const*, int, unsigned int const*, unsigned int, bool (*)(__CTFont const*, int, short*)) at /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkAdvancedTypefaceMetrics.cpp:177 #1 0x0018f85c in SkFontHost::GetAdvancedTypefaceMetrics(unsigned int, SkAdvancedTypefaceMetrics::PerGlyphInfo, unsigned int const*, unsigned int) at /Users/edisonn/src/skia-2/trunk/gyp/../src/ports/SkFontHost_mac_coretext.cpp:1567 #2 0x001698d6 in SkPDFFont::GetFontResource(SkTypeface*, unsigned short) at /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFFont.cpp:795 #3 0x00161f58 in SkPDFDevice::getFontResourceIndex(SkTypeface*, unsigned short) at /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:1566 #4 0x0015db97 in SkPDFDevice::updateFont(SkPaint const&, unsigned short, ContentEntry*) at /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:1555 #5 0x0015dfaa in SkPDFDevice::drawPosText(SkDraw const&, void const*, unsigned long, float const*, float, int, SkPaint const&) at /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:887 #6 0x0004ed62 in SkCanvas::drawPosTextH(void const*, unsigned long, float const*, float, SkPaint const&) at /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkCanvas.cpp:1894 #7 0x000b2092 in SkPicturePlayback::draw(SkCanvas&) at /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkPicturePlayback.cpp:852 #8 0x000aaf85 in SkPicture::draw(SkCanvas*) at /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkPicture.cpp:248 #9 0x0004fe04 in SkCanvas::drawPicture(SkPicture&) at /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkCanvas.cpp:2072 #10 0x000047fa in sk_tools::SimplePdfRenderer::render() at /Users/edisonn/src/skia-2/trunk/gyp/../tools/PdfRenderer.cpp:64 #11 0x00002cac in render_pdf at /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:144 #12 0x00002818 in process_input at /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:167 #13 0x00002382 in tool_main(int, char**) at /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:224 #14 0x0000299b in main at /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:236 On Fri, Jan 18, 2013 at 2:59 PM, <reed@google.com> wrote: > If we say drawText, and I pass a bad glyphID, I do not expect it to > assert. I'm OK if we draw whatever (garbarge, a box, empty), but we > should not crash in debug builds. > > getAdvancedTypefaceMetrics seems analogous to me. If we want to return > an error code, that might be fine, but I don't think this should assert. > > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >
Sign in to reply to this message.
sorry, wrong callstack, had a breakpoint set but it was not actually at a
crash
Program received signal SIGSEGV, Segmentation fault.
0x000000000052ab36 in
skia_advanced_typeface_metrics_utils::getAdvanceData<short, FT_FaceRec_*>
(fontHandle=0x3cc16e0, num_glyphs=1915, subsetGlyphIDs=0x318d2b0,
subsetGlyphIDsLength=120,
getAdvance=0x500cdb <getWidthAdvance(FT_Face, int, int16_t*)>) at
../src/core/SkAdvancedTypefaceMetrics.cpp:177
177 SkAssertResult(getAdvance(fontHandle, gId, &advance));
(gdb) up
#1 0x0000000000501912 in SkFontHost::GetAdvancedTypefaceMetrics
(fontID=100,
perGlyphInfo=(SkAdvancedTypefaceMetrics::kHAdvance_PerGlyphInfo |
SkAdvancedTypefaceMetrics::kGlyphNames_PerGlyphInfo), glyphIDs=0x318d2b0,
glyphIDsCount=120)
at ../src/ports/SkFontHost_FreeType.cpp:603
603 &getWidthAdvance));
(gdb) up
#2 0x00000000004ea8ed in SkPDFCIDFont::populate (this=0x2f44ab0,
subset=0x2d4f5a0) at ../src/pdf/SkPDFFont.cpp:1138
1138 glyphsCount));
(gdb) up
#3 0x00000000004ea2c0 in SkPDFCIDFont::SkPDFCIDFont (this=0x2f44ab0,
info=0xa59580, typeface=0x8da540, subset=0x2d4f5a0) at
../src/pdf/SkPDFFont.cpp:1062
1062 populate(subset);
(gdb) up
#4 0x00000000004ea10a in SkPDFType0Font::populate (this=0x2f2f370,
subset=0x2d4f5a0) at ../src/pdf/SkPDFFont.cpp:1043
1043 new SkPDFCIDFont(fontInfo(), typeface(), subset));
(gdb) up
#5 0x00000000004e9fdb in SkPDFType0Font::getFontSubset (this=0xa57e30,
subset=0x2d4f5a0) at ../src/pdf/SkPDFFont.cpp:1025
1025 newSubset->populate(subset);
(gdb) up
#6 0x00000000004e523c in perform_font_subsetting (catalog=0xda1cf0,
pages=..., substitutes=0x7fffffffdb68) at ../src/pdf/SkPDFDocument.cpp:49
49 entry->fFont->getFontSubset(entry->fGlyphSet);
(gdb) up
#7 0x00000000004e57cd in SkPDFDocument::emitPDF (this=0x7fffffffdaf0,
stream=0x7fffffffdbe0) at ../src/pdf/SkPDFDocument.cpp:123
123 perform_font_subsetting(fCatalog.get(), fPages, &fSubstitutes);
(gdb) up
#8 0x00000000004066ba in sk_tools::PdfRenderer::write (this=0x8b3e50,
stream=0x7fffffffdbe0) at ../tools/PdfRenderer.cpp:54
54 doc.emitPDF(stream);
(gdb) up
#9 0x0000000000404b7a in write_output (outputDir=..., inputFilename=...,
renderer=...) at ../tools/render_pdfs_main.cpp:94
94 renderer.write(&stream);
(gdb) up
#10 0x0000000000404e4c in render_pdf (inputPath=..., outputDir=...,
renderer=...) at ../tools/render_pdfs_main.cpp:146
146 success = write_output(outputDir, inputFilename, renderer);
(gdb) up
#11 0x0000000000404f91 in process_input (input=..., outputDir=...,
renderer=...) at ../tools/render_pdfs_main.cpp:167
167 if (!render_pdf(inputPath, outputDir, renderer)) {
(gdb) up
#12 0x0000000000405331 in tool_main (argc=2, argv=0x7fffffffdf18) at
../tools/render_pdfs_main.cpp:224
224 failures += process_input(inputs[i], outputDir, *renderer);
(gdb) up
#13 0x000000000040542c in main (argc=2, argv=0x7fffffffdf18) at
../tools/render_pdfs_main.cpp:236
236 return tool_main(argc, (char**) argv);
(gdb) up
Initial frame selected; you cannot go up.
On Fri, Jan 18, 2013 at 3:04 PM, Edison Nica <edisonn@google.com> wrote:
> Steve sked for the callstack
>
> #0 0x0000eb42 in SkAdvancedTypefaceMetrics::AdvanceMetric<short>*
> skia_advanced_typeface_metrics_utils::getAdvanceData<short, __CTFont
> const*>(__CTFont const*, int, unsigned int const*, unsigned int, bool
> (*)(__CTFont const*, int, short*)) at
>
/Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkAdvancedTypefaceMetrics.cpp:177
> #1 0x0018f85c in SkFontHost::GetAdvancedTypefaceMetrics(unsigned int,
> SkAdvancedTypefaceMetrics::PerGlyphInfo, unsigned int const*, unsigned int)
> at
>
/Users/edisonn/src/skia-2/trunk/gyp/../src/ports/SkFontHost_mac_coretext.cpp:1567
> #2 0x001698d6 in SkPDFFont::GetFontResource(SkTypeface*, unsigned short)
> at /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFFont.cpp:795
> #3 0x00161f58 in SkPDFDevice::getFontResourceIndex(SkTypeface*, unsigned
> short) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:1566
> #4 0x0015db97 in SkPDFDevice::updateFont(SkPaint const&, unsigned short,
> ContentEntry*) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:1555
> #5 0x0015dfaa in SkPDFDevice::drawPosText(SkDraw const&, void const*,
> unsigned long, float const*, float, int, SkPaint const&) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/pdf/SkPDFDevice.cpp:887
> #6 0x0004ed62 in SkCanvas::drawPosTextH(void const*, unsigned long, float
> const*, float, SkPaint const&) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkCanvas.cpp:1894
> #7 0x000b2092 in SkPicturePlayback::draw(SkCanvas&) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkPicturePlayback.cpp:852
> #8 0x000aaf85 in SkPicture::draw(SkCanvas*) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkPicture.cpp:248
> #9 0x0004fe04 in SkCanvas::drawPicture(SkPicture&) at
> /Users/edisonn/src/skia-2/trunk/gyp/../src/core/SkCanvas.cpp:2072
> #10 0x000047fa in sk_tools::SimplePdfRenderer::render() at
> /Users/edisonn/src/skia-2/trunk/gyp/../tools/PdfRenderer.cpp:64
> #11 0x00002cac in render_pdf at
> /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:144
> #12 0x00002818 in process_input at
> /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:167
> #13 0x00002382 in tool_main(int, char**) at
> /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:224
> #14 0x0000299b in main at
> /Users/edisonn/src/skia-2/trunk/gyp/../tools/render_pdfs_main.cpp:236
>
>
>
> On Fri, Jan 18, 2013 at 2:59 PM, <reed@google.com> wrote:
>
>> If we say drawText, and I pass a bad glyphID, I do not expect it to
>> assert. I'm OK if we draw whatever (garbarge, a box, empty), but we
>> should not crash in debug builds.
>>
>> getAdvancedTypefaceMetrics seems analogous to me. If we want to return
>> an error code, that might be fine, but I don't think this should assert.
>>
>>
https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127...
>>
>
>
Sign in to reply to this message.
Alternate fix proposed here: https://codereview.appspot.com/7128058/
Sign in to reply to this message.
updated the fix, just make sure advance is set as invalid.
Sign in to reply to this message.
On 2013/01/22 17:51:32, edisonn wrote: > updated the fix, just make sure advance is set as invalid. In normal operation (non-skp playback) this code should never fail. If we do this and there is some issue where this fails, we'll probably get bizarre output that'll probably be hard to track down. For example, we probably wouldn't have realized that .skp files are platform specific because of fonts without this code/code like this. I suggest leaving this part of the code alone and applying the fix in the CL I posted.
Sign in to reply to this message.
SkPaint paint; paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); uint16_t glyphID = 65000; canvas->drawText(&glyphID, 2, 0, 0, paint); If canvas is backed by a PDFDevice, will this trigger the old assert?
Sign in to reply to this message.
It does fail, I added that code to rrect gm, and it fails I think the right fix is to set the ignore the advance, as I did in https://codereview.appspot.com/7127056/ edisonn@edisonn:~/src/skia-5/trunk$ out/Debug/gm --match rrect Non-default runtime configuration options: drawing... rrect_clip_aa [640 480] SkBitmap::Config 0 not supported drawing... rrect_clip_bw [640 480] SkBitmap::Config 0 not supported drawing... rrect_aa [640 480] SkBitmap::Config 0 not supported drawing... rrect_bw [640 480] SkBitmap::Config 0 not supported drawing... rrect [820 710] ../src/core/SkAdvancedTypefaceMetrics.cpp:177: failed assertion "getAdvance(fontHandle, gId, &advance)" On 2013/01/22 18:14:01, reed1 wrote: > SkPaint paint; > paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); > > uint16_t glyphID = 65000; > canvas->drawText(&glyphID, 2, 0, 0, paint); > > If canvas is backed by a PDFDevice, will this trigger the old assert?
Sign in to reply to this message.
Sigh... I haven't looked at that case, but it should probably be handled higher up in the call stack. On 2013/01/22 18:48:14, edisonn wrote: > It does fail, I added that code to rrect gm, and it fails > > I think the right fix is to set the ignore the advance, as I did in > https://codereview.appspot.com/7127056/ > > > edisonn@edisonn:~/src/skia-5/trunk$ out/Debug/gm --match rrect > Non-default runtime configuration options: > drawing... rrect_clip_aa [640 480] > SkBitmap::Config 0 not supported > drawing... rrect_clip_bw [640 480] > SkBitmap::Config 0 not supported > drawing... rrect_aa [640 480] > SkBitmap::Config 0 not supported > drawing... rrect_bw [640 480] > SkBitmap::Config 0 not supported > drawing... rrect [820 710] > ../src/core/SkAdvancedTypefaceMetrics.cpp:177: failed assertion > "getAdvance(fontHandle, gId, &advance)" > > > > On 2013/01/22 18:14:01, reed1 wrote: > > SkPaint paint; > > paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); > > > > uint16_t glyphID = 65000; > > canvas->drawText(&glyphID, 2, 0, 0, paint); > > > > If canvas is backed by a PDFDevice, will this trigger the old assert?
Sign in to reply to this message.
Added a negative test gm (gmn/breakme.cpp) where we could add negative tests. This will ensure that the fix works properly across all platforms. On 2013/01/22 18:52:19, Steve VanDeBogart wrote: > Sigh... I haven't looked at that case, but it should probably be handled higher > up in the call stack. > > On 2013/01/22 18:48:14, edisonn wrote: > > It does fail, I added that code to rrect gm, and it fails > > > > I think the right fix is to set the ignore the advance, as I did in > > https://codereview.appspot.com/7127056/ > > > > > > edisonn@edisonn:~/src/skia-5/trunk$ out/Debug/gm --match rrect > > Non-default runtime configuration options: > > drawing... rrect_clip_aa [640 480] > > SkBitmap::Config 0 not supported > > drawing... rrect_clip_bw [640 480] > > SkBitmap::Config 0 not supported > > drawing... rrect_aa [640 480] > > SkBitmap::Config 0 not supported > > drawing... rrect_bw [640 480] > > SkBitmap::Config 0 not supported > > drawing... rrect [820 710] > > ../src/core/SkAdvancedTypefaceMetrics.cpp:177: failed assertion > > "getAdvance(fontHandle, gId, &advance)" > > > > > > > > On 2013/01/22 18:14:01, reed1 wrote: > > > SkPaint paint; > > > paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding); > > > > > > uint16_t glyphID = 65000; > > > canvas->drawText(&glyphID, 2, 0, 0, paint); > > > > > > If canvas is backed by a PDFDevice, will this trigger the old assert?
Sign in to reply to this message.
we could, but now we need to put that check in all the callers, and that would not be that bad, but in the future we might have other entry points to patch since now everyone needs to be aware of what is bad data, which could be a pretty lose definition, and it might vary from machine to machine. Worst case scenario, if we would keep this assert here, we need to provide another function to validate the user input. e.g. if (validate(input)) getAdvancedData(input); On Tue, Jan 22, 2013 at 2:01 PM, <edisonn@google.com> wrote: > Added a negative test gm (gmn/breakme.cpp) where we could add negative > tests. This will ensure that the fix works properly across all > platforms. > > > On 2013/01/22 18:52:19, Steve VanDeBogart wrote: > >> Sigh... I haven't looked at that case, but it should probably be >> > handled higher > >> up in the call stack. >> > > On 2013/01/22 18:48:14, edisonn wrote: >> > It does fail, I added that code to rrect gm, and it fails >> > >> > I think the right fix is to set the ignore the advance, as I did in >> > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >> > >> > >> > edisonn@edisonn:~/src/skia-5/**trunk$ out/Debug/gm --match rrect >> > Non-default runtime configuration options: >> > drawing... rrect_clip_aa [640 480] >> > SkBitmap::Config 0 not supported >> > drawing... rrect_clip_bw [640 480] >> > SkBitmap::Config 0 not supported >> > drawing... rrect_aa [640 480] >> > SkBitmap::Config 0 not supported >> > drawing... rrect_bw [640 480] >> > SkBitmap::Config 0 not supported >> > drawing... rrect [820 710] >> > ../src/core/**SkAdvancedTypefaceMetrics.cpp:**177: failed assertion >> > "getAdvance(fontHandle, gId, &advance)" >> > >> > >> > >> > On 2013/01/22 18:14:01, reed1 wrote: >> > > SkPaint paint; >> > > paint.setTextEncoding(SkPaint:**:kGlyphID_TextEncoding); >> > > >> > > uint16_t glyphID = 65000; >> > > canvas->drawText(&glyphID, 2, 0, 0, paint); >> > > >> > > If canvas is backed by a PDFDevice, will this trigger the old >> > assert? > > > > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >
Sign in to reply to this message.
This seems an incredibly small, clear change. We already call a function-ptr that is defined to return success/failure, and we make that call inside an if-block that is checking for valid glyphs already. Lets land this, since it fits in with the existing logic. If we want to explore a runtime-error mechanism for Skia (which I think might be useful), then I would definitely want to look at validating-user-input for all of our entry-points (in PDF and all other backends. lgtm
Sign in to reply to this message.
Lets move the negative test into tests/ where we already have tests that are document to ASSERT in the DEBUG build if they fail. This new test seems to be exactly of that flavor.
Sign in to reply to this message.
On 2013/01/23 14:37:20, reed1 wrote: > Lets move the negative test into tests/ where we already have tests that are > document to ASSERT in the DEBUG build if they fail. This new test seems to be > exactly of that flavor. Done, although I am not sure about naming convention: NegativeTestInvalidGlyph ?
Sign in to reply to this message.
possible just name it test_issue#### and refer to a skia bug? The comment on the test can talk about what is being tested, and how it is expected to pass/fail. (e.g. in a debug-build).
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/23 14:35:35, reed1 wrote: > This seems an incredibly small, clear change. We already call a function-ptr > that is defined to return success/failure, and we make that call inside an > if-block that is checking for valid glyphs already. > > Lets land this, since it fits in with the existing logic. If we want to explore > a runtime-error mechanism for Skia (which I think might be useful), then I would > definitely want to look at validating-user-input for all of our entry-points (in > PDF and all other backends. > > lgtm I'll make one more argument for not doing this (reverting it) and then let it drop. The code asserted because it was a case that the code was not prepared to handle. The net result for the PDF code with this change is that we will now try to draw a glyph that is out of range for the font. As far as I know, the behavior for this is not defined by the PDF spec (i.e. invalid PDF file), so it might tickle bugs in a PDF viewer... If it where to, I'd suspect an out of bounds array access, possibly leading to a segfault. I'd argue that validating the glyph list of up front - as my change does - and leaving this assert is the right thing to do. If there is a hole in the validation, this assert will trigger, alerting us to the problem. On the other hand, with the change we may not realize that under certain conditions an invalid PDF is generated.
Sign in to reply to this message.
My reading of the current *code and comments*, is that if the id is not in
subset, the id gets ignored. So I do not see what is the difference from
being ignored because "not on subset", or because "a function pointer not
recognizing it".
Edi
*// Get glyph id only when subset is NULL, or the id is in subset.*
if (!subsetGlyphIDs || (subsetIndex < subsetGlyphIDsLength &&
static_cast<uint32_t>(gId)
== subsetGlyphIDs[subsetIndex])) { - SkAssertResult(getAdvance(fontHandle,
gId, &advance)); + if (!getAdvance(fontHandle, gId, &advance)) { + advance
= kDontCareAdvance; + } ++subsetIndex; } else { advance = kDontCareAdvance;
On Wed, Jan 23, 2013 at 2:25 PM, <vandebo@chromium.org> wrote:
> On 2013/01/23 14:35:35, reed1 wrote:
>
>> This seems an incredibly small, clear change. We already call a
>>
> function-ptr
>
>> that is defined to return success/failure, and we make that call
>>
> inside an
>
>> if-block that is checking for valid glyphs already.
>>
>
> Lets land this, since it fits in with the existing logic. If we want
>>
> to explore
>
>> a runtime-error mechanism for Skia (which I think might be useful),
>>
> then I would
>
>> definitely want to look at validating-user-input for all of our
>>
> entry-points (in
>
>> PDF and all other backends.
>>
>
> lgtm
>>
>
> I'll make one more argument for not doing this (reverting it) and then
> let it drop. The code asserted because it was a case that the code was
> not prepared to handle. The net result for the PDF code with this
> change is that we will now try to draw a glyph that is out of range for
> the font. As far as I know, the behavior for this is not defined by the
> PDF spec (i.e. invalid PDF file), so it might tickle bugs in a PDF
> viewer... If it where to, I'd suspect an out of bounds array access,
> possibly leading to a segfault.
>
> I'd argue that validating the glyph list of up front - as my change does
> - and leaving this assert is the right thing to do. If there is a hole
> in the validation, this assert will trigger, alerting us to the problem.
> On the other hand, with the change we may not realize that under
> certain conditions an invalid PDF is generated.
>
>
https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127...
>
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/23 19:46:34, edisonn wrote:
> My reading of the current *code and comments*, is that if the id is not in
> subset, the id gets ignored. So I do not see what is the difference from
> being ignored because "not on subset", or because "a function pointer not
> recognizing it".
This code is just generating the advance data for the font. Over in the draw
text code, we've already used the invalid glyph id.
>
> Edi
>
>
> *// Get glyph id only when subset is NULL, or the id is in subset.*
> if (!subsetGlyphIDs || (subsetIndex < subsetGlyphIDsLength &&
> static_cast<uint32_t>(gId)
> == subsetGlyphIDs[subsetIndex])) { - SkAssertResult(getAdvance(fontHandle,
> gId, &advance)); + if (!getAdvance(fontHandle, gId, &advance)) { + advance
> = kDontCareAdvance; + } ++subsetIndex; } else { advance = kDontCareAdvance;
>
>
> On Wed, Jan 23, 2013 at 2:25 PM, <mailto:vandebo@chromium.org> wrote:
>
> > On 2013/01/23 14:35:35, reed1 wrote:
> >
> >> This seems an incredibly small, clear change. We already call a
> >>
> > function-ptr
> >
> >> that is defined to return success/failure, and we make that call
> >>
> > inside an
> >
> >> if-block that is checking for valid glyphs already.
> >>
> >
> > Lets land this, since it fits in with the existing logic. If we want
> >>
> > to explore
> >
> >> a runtime-error mechanism for Skia (which I think might be useful),
> >>
> > then I would
> >
> >> definitely want to look at validating-user-input for all of our
> >>
> > entry-points (in
> >
> >> PDF and all other backends.
> >>
> >
> > lgtm
> >>
> >
> > I'll make one more argument for not doing this (reverting it) and then
> > let it drop. The code asserted because it was a case that the code was
> > not prepared to handle. The net result for the PDF code with this
> > change is that we will now try to draw a glyph that is out of range for
> > the font. As far as I know, the behavior for this is not defined by the
> > PDF spec (i.e. invalid PDF file), so it might tickle bugs in a PDF
> > viewer... If it where to, I'd suspect an out of bounds array access,
> > possibly leading to a segfault.
> >
> > I'd argue that validating the glyph list of up front - as my change does
> > - and leaving this assert is the right thing to do. If there is a hole
> > in the validation, this assert will trigger, alerting us to the problem.
> > On the other hand, with the change we may not realize that under
> > certain conditions an invalid PDF is generated.
> >
> >
>
https://codereview.appspot.**com/7127056/%3Chttps://codereview.appspot.com/71...>
> >
Sign in to reply to this message.
On Wed, Jan 23, 2013 at 2:51 PM, <vandebo@chromium.org> wrote: > On 2013/01/23 19:46:34, edisonn wrote: > >> My reading of the current *code and comments*, is that if the id is >> > not in > >> subset, the id gets ignored. So I do not see what is the difference >> > from > >> being ignored because "not on subset", or because "a function pointer >> > not > >> recognizing it". >> > > This code is just generating the advance data for the font. Over in the > draw text code, we've already used the invalid glyph id. > Nice. Ok, then I will revert the change. > > > Edi >> > > > *// Get glyph id only when subset is NULL, or the id is in subset.* >> >> if (!subsetGlyphIDs || (subsetIndex < subsetGlyphIDsLength && >> static_cast<uint32_t>(gId) >> == subsetGlyphIDs[subsetIndex])) { - >> > SkAssertResult(getAdvance(**fontHandle, > >> gId, &advance)); + if (!getAdvance(fontHandle, gId, &advance)) { + >> > advance > >> = kDontCareAdvance; + } ++subsetIndex; } else { advance = >> > kDontCareAdvance; > > > On Wed, Jan 23, 2013 at 2:25 PM, <mailto:vandebo@chromium.org> wrote: >> > > > On 2013/01/23 14:35:35, reed1 wrote: >> > >> >> This seems an incredibly small, clear change. We already call a >> >> >> > function-ptr >> > >> >> that is defined to return success/failure, and we make that call >> >> >> > inside an >> > >> >> if-block that is checking for valid glyphs already. >> >> >> > >> > Lets land this, since it fits in with the existing logic. If we >> > want > >> >> >> > to explore >> > >> >> a runtime-error mechanism for Skia (which I think might be useful), >> >> >> > then I would >> > >> >> definitely want to look at validating-user-input for all of our >> >> >> > entry-points (in >> > >> >> PDF and all other backends. >> >> >> > >> > lgtm >> >> >> > >> > I'll make one more argument for not doing this (reverting it) and >> > then > >> > let it drop. The code asserted because it was a case that the code >> > was > >> > not prepared to handle. The net result for the PDF code with this >> > change is that we will now try to draw a glyph that is out of range >> > for > >> > the font. As far as I know, the behavior for this is not defined by >> > the > >> > PDF spec (i.e. invalid PDF file), so it might tickle bugs in a PDF >> > viewer... If it where to, I'd suspect an out of bounds array access, >> > possibly leading to a segfault. >> > >> > I'd argue that validating the glyph list of up front - as my change >> > does > >> > - and leaving this assert is the right thing to do. If there is a >> > hole > >> > in the validation, this assert will trigger, alerting us to the >> > problem. > >> > On the other hand, with the change we may not realize that under >> > certain conditions an invalid PDF is generated. >> > >> > >> > > https://codereview.appspot.****com/7127056/%3Chttps://coderev** > iew.appspot.com/7127056/ <http://codereview.appspot.com/7127056/>> > >> > >> > > > > https://codereview.appspot.**com/7127056/<https://codereview.appspot.com/7127... >
Sign in to reply to this message.
|
