From afee197c67c4ad4e80da1e92c93800b67ca083a1 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Wed, 27 Mar 2019 18:51:46 +0000 Subject: [PATCH] Rework some of the ivar offset calculations. We now correctly handle ivars that overlap with the end of what the compiler thinks is the start of the superclass and bitfields at the start of a class. --- Test/CMakeLists.txt | 1 + Test/IVarSuperclassOverlap.m | 97 ++++++++++++++++++++++++++++++++++++ Test/zeroSizedIVar.m | 2 +- ivar.c | 35 ++++++------- 4 files changed, 113 insertions(+), 22 deletions(-) create mode 100644 Test/IVarSuperclassOverlap.m diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index e6df343..8a530b1 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -41,6 +41,7 @@ set(TESTS WeakImportClass.m ivar_arc.m IVarOverlap.m + IVarSuperclassOverlap.m objc_msgSend.m msgInterpose.m NilException.m diff --git a/Test/IVarSuperclassOverlap.m b/Test/IVarSuperclassOverlap.m new file mode 100644 index 0000000..21beb2f --- /dev/null +++ b/Test/IVarSuperclassOverlap.m @@ -0,0 +1,97 @@ +#import "../objc/runtime.h" +#include "Test.h" + +@interface Space : Test +{ + uint16_t baseSmall; +} +@end + +@interface StartsWithChar : Space +{ + char c1; + char c2; + char c3; +} +@end +@interface StartsWithBitfield : Space +{ + @public + uint16_t b1:4; + uint16_t b2:4; + uint16_t b3:4; + uint16_t b4:4; + uint16_t notBitfield; +} +@end + +@implementation Space @end +@implementation StartsWithChar @end +@implementation StartsWithBitfield @end + +int main(int argc, char *argv[]) +{ + Class s = objc_getClass("Space"); + assert(s); + Class swc = objc_getClass("StartsWithChar"); + assert(swc); + Class swb = objc_getClass("StartsWithBitfield"); + assert(swb); + Ivar baseSmall = class_getInstanceVariable(s, "baseSmall"); + Ivar c1 = class_getInstanceVariable(swc, "c1"); + Ivar c2 = class_getInstanceVariable(swc, "c2"); + Ivar c3 = class_getInstanceVariable(swc, "c3"); + Ivar b1 = class_getInstanceVariable(swb, "b1"); + Ivar b2 = class_getInstanceVariable(swb, "b2"); + Ivar b3 = class_getInstanceVariable(swb, "b3"); + Ivar b4 = class_getInstanceVariable(swb, "b3"); + Ivar notBitfield = class_getInstanceVariable(swb, "notBitfield"); + assert(baseSmall); + assert(c1); + assert(c2); + assert(c3); + assert(b1); + assert(b2); + assert(b3); + StartsWithBitfield *swbi = [StartsWithBitfield new]; + + // Alternating 01 bit pattern, should catch small overwrites. + swbi->notBitfield = 0x5555; + swbi->b1 = 5; + swbi->b2 = 11; + swbi->b3 = 11; + swbi->b4 = 5; + assert(swbi->b1 == 5); + assert(swbi->b2 == 11); + assert(swbi->b3 == 11); + assert(swbi->b4 == 5); + assert(swbi->notBitfield == 0x5555); + swbi->notBitfield = 0xaaaa; + swbi->b1 = 5; + swbi->b2 = 11; + swbi->b3 = 5; + swbi->b4 = 11; + assert(swbi->notBitfield == 0xaaaa); + assert(swbi->b1 == 5); + assert(swbi->b2 == 11); + assert(swbi->b3 == 5); + assert(swbi->b4 == 11); + + ptrdiff_t baseSmallOffset = ivar_getOffset(baseSmall); +#ifdef NEW_ABI + // These should pass with the old ABI, but they don't at the moment. The + // way that they don't is not very harmful though: we just get a bit of + // redundant padding, so I don't consider a fix a very high priority. + assert(ivar_getOffset(c1) == baseSmallOffset + 2); + assert(ivar_getOffset(c2) == baseSmallOffset + 3); + assert(ivar_getOffset(c3) == baseSmallOffset + 4); + assert(ivar_getOffset(b1) == baseSmallOffset + 2); + assert(ivar_getOffset(b2) == baseSmallOffset + 2); + assert(ivar_getOffset(b3) == baseSmallOffset + 3); + assert(ivar_getOffset(b4) == baseSmallOffset + 3); + assert(ivar_getOffset(notBitfield) == baseSmallOffset + 4); +#endif + + + return 0; +} diff --git a/Test/zeroSizedIVar.m b/Test/zeroSizedIVar.m index 7b80caa..80690b4 100644 --- a/Test/zeroSizedIVar.m +++ b/Test/zeroSizedIVar.m @@ -56,7 +56,7 @@ int main() Ivar flag3 = class_getInstanceVariable(bitfield, "flag3"); assert(flag3); assert(ivar_getOffset(flag3) == sizeof(id)); - assert(ivar_getOffset(flag3) + sizeof(int) <= class_getInstanceSize(bitfield)); + assert(ivar_getOffset(flag3) + sizeof(BOOL) <= class_getInstanceSize(bitfield)); bitfield = objc_getClass("BitfieldTest2"); flag1 = class_getInstanceVariable(bitfield, "flag1"); diff --git a/ivar.c b/ivar.c index a1311b0..92af800 100644 --- a/ivar.c +++ b/ivar.c @@ -65,22 +65,13 @@ PRIVATE void objc_compute_ivar_offsets(Class class) // If the first instance variable had any alignment padding, then we need // to discard it. We will recompute padding ourself later. long next_ivar = ivar_start; - long last_offset = -1; + long last_offset = LONG_MIN; + long last_size = 0; long last_computed_offset = -1; size_t refcount_size = isGCEnabled ? 0 : sizeof(uintptr_t); for (i = 0 ; i < class->ivars->count ; i++) { struct objc_ivar *ivar = ivar_at_index(class->ivars, i); - // The compiler may generate negative offsets for the first - // instance variable, because the last instance variable falls - // within the space allocated for the last instance variable. - // We are completely recomputing the instance size, so we - // simply discard this and recompute the padding ourselves. - if (*ivar->offset < 0) - { - assert(i == 0); - *ivar->offset = ivar_start; - } // Clang 7 and 8 have a bug where the size of _Bool is encoded // as 0, not 1. Silently fix this up when we see it. if (ivar->size == 0 && ivar->type[0] == 'B') @@ -93,18 +84,20 @@ PRIVATE void objc_compute_ivar_offsets(Class class) // then we will need to ensure that we are properly aligned again. long ivar_size = ivar->size; // Bitfields have the same offset - the base of the variable - // that contains them. If we are in a bitfield, - if (*ivar->offset == last_offset) - { - *ivar->offset = last_computed_offset; - } - else + // that contains them. If we are in a bitfield, then we need + // to make sure that we don't add any displacement from the + // previous value. + if (*ivar->offset < last_offset + last_size) { - last_offset = *ivar->offset; - *ivar->offset = next_ivar; - last_computed_offset = *ivar->offset; - next_ivar += ivar_size; + *ivar->offset = last_computed_offset + (*ivar->offset - last_offset); + ivar_size = 0; + continue; } + last_offset = *ivar->offset; + *ivar->offset = next_ivar; + last_computed_offset = *ivar->offset; + next_ivar += ivar_size; + last_size = ivar->size; size_t align = ivarGetAlign(ivar); if ((*ivar->offset + refcount_size) % align != 0) {