Compile warning: static string out-of-bounds access

[GoogleCodeExporter]

On a fresh chromium checkout, a build with G++ (mine is 4.9.1 from Debian) 
gives the following warning:

CXX 
obj/third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdfapi.fpdf_parser_parser.o
../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp: 
In member function ‘CPDF_Object* 
CPDF_SyntaxParser::GetObject(CPDF_IndirectObjects*, FX_DWORD, FX_DWORD, 
FX_INT32, PARSE_CONTEXT*, FX_BOOL)’:
../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:219
9:98: warning: offset outside bounds of constant string
                     pDict->SetAt(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() - 1), pObj);

Original issue reported on code.google.com by wi...@chromium.org on 21 Oct 2014 at 7:09

[GoogleCodeExporter]

Presumably |CFX_ByteString::operator FX_LPCSTR| has a path to return "":
        return m_pData ? m_pData->m_String : "";

meaning that |((FX_LPCSTR)key) + 1| is the same as |""[1]| which is outside 
bounds. Checking for a zero-length key probably resolves the issue.

Original comment by tsepez@chromium.org on 21 Oct 2014 at 5:56

[GoogleCodeExporter]

In actuality, it doesn't look like the call
    2187:    key = PDF_NameDecode(key);

can return a zero-length value unless passed a zero-length input, and this is 
previously checked at:
    2168:    if (key.IsEmpty()) {

but g++ can't know that.  So the issue doesn't manifest itself in practice; for 
now it will be necessary to suppress that warning since I expect there are 
other places where you'll hit this. 

I'm guessing g++ 4.8.2 doesn't have this warning, or we'd hit it ourselves.

Original comment by tsepez@chromium.org on 21 Oct 2014 at 6:39

[GoogleCodeExporter]

Issue 47 has been merged into this issue.

Original comment by tsepez@chromium.org on 21 Oct 2014 at 6:47

[GoogleCodeExporter]

Speculative CL at https://codereview.chromium.org/665223003/

Original comment by tsepez@chromium.org on 21 Oct 2014 at 7:56

[GoogleCodeExporter]

Landed at f3a5d3ddf7b492e5e8f64cb2ee98a53b5520499d.

Please let me know if this resolves your build issue.  If you're using a 
chromium checkout, you'll either have to wait for a DEPS roll to get this 
revision, or you can add a:
   'custom_vars': {
      'pdfium_revision': ''
    },
to your "src" solution in your .gclient file and gclient sync if you want to 
always be at the latest (potentially broken) PDFium version.

Original comment by tsepez@chromium.org on 29 Oct 2014 at 10:37

[GoogleCodeExporter]

I'm going to mark this fixed, please re-open if the change landed in c#5 didn't 
resolve the issue.

Original comment by tsepez@chromium.org on 5 Nov 2014 at 6:31

• Changed state: Fixed

[GoogleCodeExporter]

Any idea when the next DEPS roll is going to happen?

Original comment by chiran...@chromium.org on 5 Nov 2014 at 6:52

[GoogleCodeExporter]

I've checked the build with gcc 4.9.1 and with the CL applied I've the 
following warning:

c++ -MMD -MF 
obj/third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdfapi.fpdf_parser_parser.o
.d -DFOXIT_CHROME_BUILD -D_FXFT_VERSION_=2501 -D_FPDFSDK_LIB -D_NO_GDIPLUS_ 
-DOPJ_STATIC -DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DDISABLE_NACL 
-D_FX_CPU_=_FX_X64_ -DCHROMIUM_BUILD -DCOMPONENT_BUILD -DTOOLKIT_VIEWS=1 
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1 
-DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 
-DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP 
-DUSE_XI2_MT=2 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS 
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DUSE_UDEV 
-DDONT_EMBED_BUILD_METADATA -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 
-DCLD_VERSION=2 -DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1 
-DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 
-DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1 
-DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_LOAD_COMPLETION_HACKS=1 
-DUSE_GLIB=1 -DUSE_NSS=1 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 
-Igen -fstack-protector --param=ssp-buffer-size=4 -pthread -fno-strict-aliasing 
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe 
-fPIC -Wno-unused-local-typedefs -fPIC -Wno-format -Wno-unused-result -m64 
-march=x86-64 -O2 -fno-ident -fdata-sections -ffunction-sections 
-funwind-tables -fno-exceptions -fno-rtti -fno-threadsafe-statics 
-fvisibility-inlines-hidden -Wno-deprecated -std=gnu++11 -Wno-narrowing 
-Wno-literal-suffix  -c 
../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp -o 
obj/third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdfapi.fpdf_parser_parser.o
../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp: 
In member function ‘CPDF_Object* 
CPDF_SyntaxParser::GetObjectByStrict(CPDF_IndirectObjects*, FX_DWORD, FX_DWORD, 
FX_INT32, PARSE_CONTEXT*)’:
../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:238
5:97: warning: offset outside bounds of constant string
                 pDict->AddValue(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() - 1), pObj);

And here is the CL to fix the warning: https://codereview.chromium.org/716103002

Original comment by pgal.u-s...@partner.samsung.com on 12 Nov 2014 at 9:28