New issue
Advanced search Search tips
Starred by 6 users
Status: Accepted
Owner:
Cc:
Area: AndroidFramework
Priority: Medium
Type: Defect



Sign in to add a comment
skia and -D_FORTIFY_SOURCE=2 do not play nice
Reported by nnk@google.com, Apr 30 2013 Back to list

Please see https://android-review.googlesource.com/57759 for an Android commit for this issue. 

At first glance, it seems like fBeginningOfData should be changed from something other than a char, perhaps a "char *", or ... Otherwise, any FORTIFY_SOURCE=2 code is going to incorrectly infer the size of the SkString elements.

----- Begin -----
skia: Don't use -D_FORTIFY_SOURCE=2

SkString uses a dynamic growing mechanism for storing strings.
This dynamic mechanism confuses GCC, which thinks it's operating
with a single character. This causes crashes when we use
Android's -D_FORTIFY_SOURCE=2 extensions.

Consider the following code:
  SkLanguage SkLanguage::getParent() const {
      SkASSERT(fInfo != NULL);
      const char* tag = fInfo->fTag.c_str();
      SkASSERT(tag != NULL);
      // strip off the rightmost "-.*"
      char* parentTagEnd = strrchr(tag, '-');
  [DELETED]
  }

fInfo->fTag is a SkString, which is declared as:

  class SkString {
  public:
  [DELETED]
      size_t      size() const { return (size_t) fRec->fLength; }
      const char* c_str() const { return fRec->data(); }
      char operator[](size_t n) const { return this->c_str()[n]; }
  [DELETED]
  private:
      struct Rec {
      public:
          size_t      fLength;
          int32_t     fRefCnt;
          char        fBeginningOfData;
          char* data() { return &fBeginningOfData; }
          const char* data() const { return &fBeginningOfData; }
      };
      Rec* fRec;
  [DELETED]
  };

When "fInfo->fTag.c_str();" is executed in "SkLanguage::getParent()", it
returns a pointer to fBeginningOfData, which is a single char. The compiler believes that "tag" always points to an element of size 1. When Android's strrchr() fortify extensions are called, we trigger an abort, because strrchr() detects an attempt to read beyond the one character
fBeginningOfData.
 
Project Member Comment 1 by scro...@google.com, May 1 2013
Cc: djsollen@google.com reed@google.com
It appears SkString has always returned a pointer to a single char in data(). Mike do you remember why we point to a single char instead of casting the address to char*?

nnk@, if you change skia the way you'd like, does it fix the problem?
Comment 2 by nnk@google.com, May 1 2013
I went ahead and implemented a similar change on Android:

https://android-review.googlesource.com/57816

which is essentially:

diff --git a/include/core/SkString.h b/include/core/SkString.h
index 3fa367b..0ccead1 100644
--- a/include/core/SkString.h
+++ b/include/core/SkString.h
@@ -155,8 +155,8 @@ private:
         int32_t     fRefCnt;
         char        fBeginningOfData;
 
-        char* data() { return &fBeginningOfData; }
-        const char* data() const { return &fBeginningOfData; }
+        char* data() { return ((char *) this) + offsetof(Rec, fBeginningOfData); }
+        const char* data() const { return ((const char *) this) + offsetof(Rec, fBeginningOfData); }
     };
     Rec* fRec;

and it seems to have fixed my problems. Of course, I haven't done a lot of testing on this change.

This change *should* be binary compatible, and should basically do the same thing that the previous code did.
Project Member Comment 3 by reed@google.com, May 1 2013
Can we instead remove fBeginningOfData entirely, and say:

char* data() { return ((char*)this + sizeof(*this)); }

I think that more closely matches how we do this sort of trick in other
parts of skia.
Project Member Comment 4 by scro...@google.com, May 1 2013
Status: Accepted
Sounds good to me. I'll make the change in Skia.
Comment 5 by nnk@google.com, May 1 2013
RE: comment #3:

I verified that the following patch also resolves my problems:

diff --git a/include/core/SkString.h b/include/core/SkString.h
index 743f078..95bb051 100644
--- a/include/core/SkString.h
+++ b/include/core/SkString.h
@@ -207,10 +207,9 @@ private:
     public:
         size_t      fLength;
         int32_t     fRefCnt;
-        char        fBeginningOfData;
 
-        char* data() { return &fBeginningOfData; }
-        const char* data() const { return &fBeginningOfData; }
+        char* data() { return (char*)this + sizeof(*this); }
+        const char* data() const { return (const char*)this + sizeof(*this); }
     };
     Rec* fRec;
 
diff --git a/src/core/SkString.cpp b/src/core/SkString.cpp
index 3f9a99f..fb30693 100644
--- a/src/core/SkString.cpp
+++ b/src/core/SkString.cpp
@@ -184,8 +184,8 @@ char* SkStrAppendFixed(char string[], SkFixed x) {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-// the 3 values are [length] [refcnt] [terminating zero data]
-const SkString::Rec SkString::gEmptyRec = { 0, 0, 0 };
+// the 2 values are [length] [refcnt]
+const SkString::Rec SkString::gEmptyRec = { 0, 0 };
 
 #define SizeOfRec()     (gEmptyRec.data() - (const char*)&gEmptyRec)
 

Comment 6 by nnk@google.com, May 1 2013
Actually, I just realized a problem with comment #3 / comment #5.  By getting rid of fBeginningOfData, any call to gEmptyRec.data() will return a pointer to unallocated memory.  It's only by luck that gEmptyRec happens to be surrounded by other, all zero objects.

Given this, I think the solution in comment #2 is better.
Project Member Comment 7 by reed@google.com, May 1 2013
doh, you're right that gEmptyRec is now too small (and won't have a terminating 0).

Can you help us reproduce the FORTIFY message in our skia build world? We would love to see these errors ourselves (and allow us to not regress w/ future changes).
Project Member Comment 8 by scro...@google.com, May 2 2013
Cc: -djsollen@google.com scro...@google.com bore...@google.com
Owner: djsollen@google.com
Assigning to Derek to try building Skia on Android with this flag.
Comment 9 by nnk@google.com, Dec 8 2013
A version of Android with -D_FORTIFY_SOURCE=2 defined everywhere, except for skia, has been released. Do you still need help reproducing this error? 

Reproducing this error on Android should be as simple as changing "1" to "2" in https://android-review.googlesource.com/#/c/57759/1/Android.mk .
Project Member Comment 10 by djsollen@google.com, Dec 9 2013
Status: Fixed
Project Member Comment 11 by djsollen@google.com, Jan 23 2014
Status: Accepted
We had to revert the fix in Android due to it crashing the device on startup. More investigation needs to be on SkString in order to fix this.
Project Member Comment 12 by djsollen@google.com, Jan 23 2014
Labels: OpSys-Android
Project Member Comment 13 by hcm@google.com, Aug 28 2014
Labels: Area-AndroidFramework
Project Member Comment 14 by djsollen@google.com, Oct 2 2014
I have FORTIFY_SOURCE=2 enabled on our linux build and even without the change to SkString all our tests are passing (including one I wrote to emulate SkLanguage). HOwever, I can't get the NDK to honor the define so it seems we will not be able to validate the test in our upstream framework.

I'll work on verifying that my new test does indeed fail within the android framework, then I'll verify the fix in #2, commit, and close this bug
Project Member Comment 15 by hcm@google.com, Dec 7 2015
Labels: Hotlist-Fixit
Sign in to add a comment