Fix binding of null pointer dereference to reference type

core/fxcrt/include/fx_basic.h

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #ifndef CORE_FXCRT_INCLUDE_FX_BASIC_H_
 #define CORE_FXCRT_INCLUDE_FX_BASIC_H_
 
 #include <algorithm>
+#include <cstdlib>
 #include <memory>
 
 #include "core/fxcrt/include/fx_memory.h"
 #include "core/fxcrt/include/fx_stream.h"
 #include "core/fxcrt/include/fx_string.h"
 #include "core/fxcrt/include/fx_system.h"
 
 class CFX_BinaryBuf {
  public:
   CFX_BinaryBuf();
@@ skipping 249 matching lines @@
   int GetSize() const { return m_nSize; }
 
   int GetUpperBound() const { return m_nSize - 1; }
 
   FX_BOOL SetSize(int nNewSize) { return CFX_BasicArray::SetSize(nNewSize); }
 
   void RemoveAll() { SetSize(0); }
 
   const TYPE GetAt(int nIndex) const {
     if (nIndex < 0 || nIndex >= m_nSize) {
-      return (const TYPE&)(*(volatile const TYPE*)NULL);
+      abort();
     }
     return ((const TYPE*)m_pData)[nIndex];
   }
 
   FX_BOOL SetAt(int nIndex, TYPE newElement) {
     if (nIndex < 0 || nIndex >= m_nSize) {
       return FALSE;
     }
     ((TYPE*)m_pData)[nIndex] = newElement;
     return TRUE;
   }
 
   TYPE& ElementAt(int nIndex) {
     if (nIndex < 0 || nIndex >= m_nSize) {
-      return *(TYPE*)NULL;
+      abort();

[dsinclair, 2016/05/16 17:41:02]

I don't think this is safe to do. We can't assume the current library isn't
depending on this behaviour and we're now going to start aborting where we'd
have continued previously.

[hans, 2016/05/16 17:48:26]

Getting a reference bound to null as the return value sounds like a horrible
thing to depend on :-)

I think the intention of the code is clearly to abort here.

Grepping the source for callers at ElementAt, I don't see anything that would
crash immediately on a reference-to-null.

[Tom Sepez, 2016/05/16 17:49:18]

But .. they wanted to crash here, so this should be fine. Unfortunately, we
wouldn't do so in many platforms since the optimizer can assume that null derefs
never occur, and hence remove this deliberate null deref.

[Nico, 2016/05/16 17:49:34]

References pointing to null have undefined behavior, and compilers do exploit
this. Since this is an inline function in a header file, the compiler could do
arbitrary things at calling points if it knows at compile time that nIndex is 0
or >= m_nSize. So that needs to be fixed anyhow. Now this'll provide a clean
crash report when that happens and we'll learn about it. It might cause
short-term pain, but it's the right change going forward.

(Maybe this should use something like blink's IMMEDIATE_CRASH instead of abort
though, not sure.)

     }
     return ((TYPE*)m_pData)[nIndex];
   }
 
   const TYPE* GetData() const { return (const TYPE*)m_pData; }
 
   TYPE* GetData() { return (TYPE*)m_pData; }
 
   FX_BOOL SetAtGrow(int nIndex, TYPE newElement) {
     if (nIndex < 0)
@@ skipping 756 matching lines @@
   FX_FLOAT c;
   FX_FLOAT d;
   FX_FLOAT e;
   FX_FLOAT f;
   FX_FLOAT g;
   FX_FLOAT h;
   FX_FLOAT i;
 };
 
 #endif  // CORE_FXCRT_INCLUDE_FX_BASIC_H_