There is also an unrelated UBSAN report for the same PDF, which I think is bogus.

../../core/fxcrt/fx_arabic.cpp:143:32: runtime error: index 1602 out of bounds for type 'const (anonymous namespace)::FX_ARBFORMTABLE [180]'
    #0 0x1a7dd47 in (anonymous namespace)::GetArabicFormTable(wchar_t) ../../core/fxcrt/fx_arabic.cpp:143:32

This fixes the report.

-  return g_FX_ArabicFormTables + unicode - 0x622;
+  return g_FX_ArabicFormTables + (unicode - 0x622);

In debug builds, it's failing the "iClsCur <= FX_BIDICLASS_BN" assertion.

If the code actually checks the value instead of doing an assert, then it needs to do the same in ResolveNeutrals() and ResolveImplicit() for the given test case. Not sure if that's the right answer or if the code should do something different in Classify() in the first place.

We should add unit tests for FX_BidiLine(), and maybe even a fuzzer.

I don't know what the bug is exactly, or rather how it should be fixed, but I can explain where the bug is.

There are three characters in the string. In Classify, each is assigned m_BidiClass (later referred to by iCls*) with the following operation.

(cur.char_props() & FX_BIDICLASSBITSMASK) >> FX_BIDICLASSBITS

https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxcrt/fx_bidi.cpp?l=315&rcl=36b3d19281e2911a97d6ce84538a3ae575ac38a7

And char_props() is read in earlier as dwProps in AppendChar. FX_GetUnicodeProperties just looks up the value in a large table.

uint32_t dwProps = FX_GetUnicodeProperties(wch);

https://cs.chromium.org/chromium/src/third_party/pdfium/xfa/fgas/layout/cfx_txtbreak.cpp?l=228&rcl=36b3d19281e2911a97d6ce84538a3ae575ac38a7

And the second character, \x1D returns char_props() that results in m_BidiClass == 13, which is outside of the asserted range of 0-10 (and 1-5 later).

Now the interesting part is why three characters are necessary to trigger the bug, rather than just that character.

The first character gets into the code path that calls FX_BidiLine in EndBreak_BidiLine.

https://cs.chromium.org/chromium/src/third_party/pdfium/xfa/fgas/layout/cfx_txtbreak.cpp?l=320&rcl=36b3d19281e2911a97d6ce84538a3ae575ac38a7

The second character is explained above, but what about the third? In EndBreak_BidiLine, the following code.

    size_t iBidiNum = 0;
    for (size_t i = 0; i < static_cast<size_t>(iCount); ++i) {
      pTC = &chars[i];
      pTC->m_iBidiPos = static_cast<int32_t>(i);
      if (pTC->GetCharType() != FX_CHARTYPE_Control)
        iBidiNum = i;
      if (i == 0)
        pTC->m_iBidiLevel = 1;
    }
    FX_BidiLine(&chars, iBidiNum + 1); // <-- iBidiNum + 1 == count of chars FX_BidiLine processes

The effect of the above loop is that if the last character of the line is of type FX_CHARTYPE_Control (which \x1D is), then it is not processed by FX_BidiLine. So an additional non-FX_CHARTYPE_Control character is necessary.

I don't know why it's only ignored when it's last, or if that's the bug.

Minor addition: multiple FX_CHARTYPE_Control characters in last position are also ignored.

Using different characters triggers a similar ASAN report in CFX_BidiLine::ResolveNeutrals.

==646==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000035bd04 at pc 0x000000b306f2 bp 0x7ffd6e5117a0 sp 0x7ffd6e511798
READ of size 4 at 0x00000035bd04 thread T0
SCARINESS: 27 (4-byte-read-global-buffer-overflow-far-from-bounds)
    #0 0xb306f1 in (anonymous namespace)::CFX_BidiLine::ResolveNeutrals(std::__1::vector<CFX_Char, std::__1::allocator<CFX_Char> >*, unsigned long) ../../core/fxcrt/fx_bidi.cpp:418:17
...
0x00000035bd04 is located 28 bytes to the left of global variable '(anonymous namespace)::gc_FX_BidiNeutralStates' defined in '../../core/fxcrt/fx_bidi.cpp:211:15' (0x35bd20) of size 120
0x00000035bd04 is located 12 bytes to the right of global variable '(anonymous namespace)::gc_FX_BidiNeutralActions' defined in '../../core/fxcrt/fx_bidi.cpp:219:15' (0x35bc80) of size 120

Deleted: chromium-839695-1.pdf 310 bytes

chromium-839695-1.pdf 310 bytes Download

I propose the following patch.

--- a/xfa/fgas/layout/cfx_txtbreak.cpp
+++ b/xfa/fgas/layout/cfx_txtbreak.cpp
@@ -110,6 +110,10 @@ CFX_BreakType CFX_TxtBreak::AppendChar_Control(CFX_Char* pCurChar) {
       case L'\f':
         dwRet = CFX_BreakType::Page;
         break;
+      case 0x1C:
+      case 0x1D:
+      case 0x1E:
+      case 0x85:
       case 0x2029:
         dwRet = CFX_BreakType::Paragraph;
         break;

There are 7 unicode characters with Bidi_Class Paragraph Separator. (B in 5th column.) pdfium refers to them as FX_BIDICLASS_B (13).

ftp://ftp.unicode.org/Public/UNIDATA/UnicodeData.txt

https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxcrt/fx_bidi.cpp?l=34&rcl=967aa0793c0b0cf2722ec8720e9d797266a9fde7

pdfium currently handles 0x2029, 0x0A and 0x0D (the latter two are m_wParagraphBreakChar in the above switch) which all cause the layout code to break after those characters. And then the loop mentioned in the previous post causes those pending characters to not be processed by FX_BidiLine. The other 4 characters are not handled, don't break, are processed, and then cause the bug (or hit the assert).

Note that 0x85 doesn't actually trigger the bug with the above PDFs. It makes it not even reach any of this code. I assume it causes an earlier parser to fail somewhere.

PS. The same code is in CFX_RTFBreak::AppendChar_Control.

I wasn't able to repro the issue with the provided files but making a fuzzer found the assert pretty quickly. I added a test case which also triggers in:

https://pdfium-review.googlesource.com/c/pdfium/+/32191

Unfortunately my patch doesn't quite fix the actual bug. It works for the attached PDFs, but there are other cases for which it doesn't. I didn't notice that m_wParagraphBreakChar is either 0x0A or 0x0D (set to 0x0A by default, and then changed to the first encountered in the string), so the other isn't matched.

--- a/xfa/fgas/layout/cfx_txtbreak.cpp
+++ b/xfa/fgas/layout/cfx_txtbreak.cpp
@@ -110,6 +110,12 @@ CFX_BreakType CFX_TxtBreak::AppendChar_Control(CFX_Char* pCurChar) {
       case L'\f':
         dwRet = CFX_BreakType::Page;
         break;
+      case 0x0A:
+      case 0x0D:
+      case 0x1C:
+      case 0x1D:
+      case 0x1E:
+      case 0x85:
       case 0x2029:
         dwRet = CFX_BreakType::Paragraph;
         break;

The purpose of m_wParagraphBreakChar is not clear to me. With the above patch, it appears useless.

Have you tried a build with XFA enabled? I can reproduce with the above files and pdfium_test.

Yes, I tried with XFA enabled.

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/5e0b271b69355b5692b6afd1cd2c04d08c3b380c

commit 5e0b271b69355b5692b6afd1cd2c04d08c3b380c
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Thu May 10 21:21:05 2018

Fixup ASSERT in Bidi handling; Add bidi fuzzer.

This CL converts several asserts in the FX_Bidi code to continue instead
of asserting in the face of unexpected input.

A BIDI fuzzer has been added as well.

Bug:  chromium:839695 
Change-Id: If61f822bde7442c008d50be58f7cecffb6e5d658
Reviewed-on: https://pdfium-review.googlesource.com/32191
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/testing/libfuzzer/BUILD.gn
[add] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/xfa/fgas/layout/cfx_txtbreak_unittest.cpp
[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/core/fxcrt/fx_bidi.cpp
[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/xfa/fgas/layout/cfx_break.h
[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/xfa/fgas/layout/cfx_rtfbreak_unittest.cpp
[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/xfa/fgas/layout/cfx_rtfbreak.h
[modify] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/BUILD.gn
[add] https://crrev.com/5e0b271b69355b5692b6afd1cd2c04d08c3b380c/testing/libfuzzer/pdf_bidi_fuzzer.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/777a737b6e980720bebdb535c7da9c5788999551

commit 777a737b6e980720bebdb535c7da9c5788999551
Author: pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri May 11 03:27:10 2018

Roll src/third_party/pdfium/ 5ad45e2f6..5e0b271b6 (1 commit)

https://pdfium.googlesource.com/pdfium.git/+log/5ad45e2f68bb..5e0b271b6935

$ git log 5ad45e2f6..5e0b271b6 --date=short --no-merges --format='%ad %ae %s'
2018-05-10 dsinclair Fixup ASSERT in Bidi handling; Add bidi fuzzer.

Created with:
  roll-dep src/third_party/pdfium
BUG= chromium:839695 


The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=dsinclair@chromium.org

Change-Id: I8dd271c6d77f96e369f68a2be87e978a7211f9c4
Reviewed-on: https://chromium-review.googlesource.com/1054623
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#557777}
[modify] https://crrev.com/777a737b6e980720bebdb535c7da9c5788999551/DEPS

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b997d59c3a690405703ac5a4e69503d5325381f9

commit b997d59c3a690405703ac5a4e69503d5325381f9
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon May 14 14:53:47 2018

Add pdf_bidi_fuzzer build

Bug:  chromium:839695 
Change-Id: I062d9a97559a4d26062e8a4932527765f06025a4
Reviewed-on: https://chromium-review.googlesource.com/1050584
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558306}
[modify] https://crrev.com/b997d59c3a690405703ac5a4e69503d5325381f9/pdf/pdfium/fuzzers/BUILD.gn

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Hi pdknsk@, the Chrome VRP panel decided this should be tracked as Medium severity, and awarded $1,000 for this report. Thanks!