Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-6468] inheriting from class with synthesized Codable implementation creates invalid code #49018

Closed
swift-ci opened this issue Nov 24, 2017 · 20 comments

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Nov 24, 2017

Previous ID SR-6468
Radar rdar://problem/35699277
Original Reporter derammo (JIRA User)
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment

swift-4.1-DEVELOPMENT-SNAPSHOT-2017-11-23-a.xctoolchain

also happens in Xcode 9.1 as released by Apple:

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, Codable
Assignee @slavapestov
Priority Medium

md5: 44a3722f047bd288f0a7c1c8573bee7e

is duplicated by:

  • SR-6507 Encodable in combination with class inheritance and computed properties generates unexpected behaviour
  • SR-7090 Crashing with EXC_BAD_ACCESS after decoding Codable subclass
  • SR-7156 Synthesized instance methods result in vtable-related crashes

Issue Description:

Inheriting from a class that has some of its "Codable" protocol implementation synthesized automatically can lead to invalid code. Specifically, the derived class ends up with code that accesses out of the bounds of the table of methods when trying to invoke a method on an instance of the derived class.

I verified this by disassembly. Instead of fetching the address of the first getter, it fetches a small number (which seems to be the table size) that is located 8 bytes to the left of the correct information. Then it calls this address, which obviously faults.

In this example, you see if it fetching the 0x18 (presumably table size) instead of the valid address of the getter 0x0106bdf460, which is stored right after it.

(lldb) dis -l

-> 10      func testInheritedGeneratedInitializerFromCodableSynthesizer() {
-> 11          let instance = TestPartiallySynthesizedDerivedClass(arg1: 1, arg2: 2)
-> 12          let getting = instance.derivedMember

testUnit`testCompiler.testInheritedGeneratedInitializerFromCodableSynthesizer():
->  0x106bb6af9 <+41>: movq   (%rax), %rsi
    0x106bb6afc <+44>: movq   0xb0(%rsi), %rsi
    0x106bb6b03 <+51>: movq   %rax, %r13
    0x106bb6b06 <+54>: movq   %rax, -0x18(%rbp)
    0x106bb6b0a <+58>: callq  *%rsi
(lldb) nexti
(lldb) register read rsi
     rsi = 0x0000000106c46550  type metadata for testUnit.TestPartiallySynthesizedDerivedClass
(lldb) memory read 0x106C46600
0x106c46600: 18 00 00 00 00 00 00 00 60 f4 bd 06 01 00 00 00  ........`.......
0x106c46610: b0 f4 bd 06 01 00 00 00 30 f5 bd 06 01 00 00 00  ........0.......
(lldb) dis -a 0106bdf460
testUnit`_T08testUnit36TestPartiallySynthesizedDerivedClassC13derivedMemberSbvg:
    0x106bdf460 <+0>:  pushq  %rbp
    0x106bdf461 <+1>:  movq   %rsp, %rbp
    0x106bdf464 <+4>:  subq   $0x40, %rsp
    0x106bdf468 <+8>:  movq   %r13, -0x8(%rbp)
    0x106bdf46c <+12>: movq   %r13, %rax
    0x106bdf46f <+15>: addq   $0x20, %rax
    0x106bdf473 <+19>: xorl   %ecx, %ecx
    0x106bdf475 <+21>: movl   %ecx, %edx
    0x106bdf477 <+23>: leaq   -0x20(%rbp), %rsi
    0x106bdf47b <+27>: movq   %rax, %rdi
    0x106bdf47e <+30>: movq   %rsi, -0x28(%rbp)
    0x106bdf482 <+34>: movq   %rdx, -0x30(%rbp)
    0x106bdf486 <+38>: movq   -0x30(%rbp), %rcx
    0x106bdf48a <+42>: movq   %r13, -0x38(%rbp)
    0x106bdf48e <+46>: callq  0x106c278de               ; symbol stub for: swift_beginAccess
    0x106bdf493 <+51>: movq   -0x38(%rbp), %rax
    0x106bdf497 <+55>: movb   0x20(%rax), %al
    0x106bdf49a <+58>: movq   -0x28(%rbp), %rdi
    0x106bdf49e <+62>: movb   %al, -0x39(%rbp)
    0x106bdf4a1 <+65>: callq  0x106c2790e               ; symbol stub for: swift_endAccess
    0x106bdf4a6 <+70>: movb   -0x39(%rbp), %al
    0x106bdf4a9 <+73>: addq   $0x40, %rsp
    0x106bdf4ad <+77>: popq   %rbp
    0x106bdf4ae <+78>: retq   

The problem is reliably reproduced but extremely specific. Adding explicit support for Codable in the base class makes it go away. Adding explicit support for Codable in the derived class (by adding decode(from) also) also makes it go away. The classes must be in a separate file from the caller for this to fail.

The attached files reproduce the problem 100% of the time as a unit test.

@belkadan
Copy link
Contributor

belkadan commented Nov 27, 2017

@itaiferber, any idea what's going on here? I know we've had problems here but I didn't expect crashes!

@itaiferber
Copy link
Contributor

itaiferber commented Nov 27, 2017

@belkadan I'm not sure, to be honest. This is the first I've seen of this, so I'll investigate. It looks like inheriting the superclass's method is somehow invalid? That seems really strange to me.

@itaiferber
Copy link
Contributor

itaiferber commented Nov 27, 2017

@swift-ci Create

@swift-ci
Copy link
Collaborator Author

swift-ci commented Nov 27, 2017

Comment by Ammo Goettsch (JIRA)

I saw it again in another place in the code yesterday, where I was inheriting a class. Once I ported the base class code to use Codable (with some synthesized code), it started calling the wrong method in the derived class in a test (i.e. wrong offset again, this time hitting an actual method rather than the end of the table.) So I don't think it as rare as I thought.

@belkadan
Copy link
Contributor

belkadan commented Nov 27, 2017

Just to double-check, you are rebuilding the test after changing the codable classes, right?

@swift-ci
Copy link
Collaborator Author

swift-ci commented Nov 27, 2017

Comment by Ammo Goettsch (JIRA)

Oh yes: clean, exit Xcode, rm -rf ~/Library/Developer/Xcode/DerivedData

I even sometimes reboot to clear out /var/folders where Xcode makes a mess... but I can't swear I have done that for this test. The attached test code should produce the problem though for you. Let me know if it does not repro and I can work with you to find the settings I might be using differently or send you a project file.

@itaiferber
Copy link
Contributor

itaiferber commented Mar 16, 2018

@belkadan Thanks for consolidating! Who might best be able to help me investigate what's happening with the vtable here (or help point in the right direction)? Encodable synthesis isn't doing anything special and follows the same model as Hashable and Equatable.

@lorentey
Copy link
Member

lorentey commented Mar 16, 2018

Encodable is special in that it is currently the only protocol for which we synthesize methods in class types. I'm also interested in finding a good solution to this – I ran into the same issue when I tried adding limited support for classes for Hashable synthesis in PR #15122.

@belkadan
Copy link
Contributor

belkadan commented Mar 16, 2018

I shoved @slavapestov at one of the dups but I don't know if he's started looking yet. In our brief conversations about it we haven't thought of anything better than "if you use a class, you must validate its Hashable conformance".

@itaiferber
Copy link
Contributor

itaiferber commented Mar 16, 2018

@belkadan What do you mean by "validate its conformance"? Don't we already have passes that validate the decls? Or is this at a different layer/level?

@belkadan
Copy link
Contributor

belkadan commented Mar 16, 2018

I mean it's not good enough to know that the class declares conformance to Hashable; we need to actually go check that it does so correctly, because that's what forces the new overridable declarations to be synthesized.

@lorentey
Copy link
Member

lorentey commented Mar 16, 2018

FWIW, I went for a little Mr Magoo-style adventure in the type checker, and I was excited to stumble upon TypeChecker::handleExternalDecl. Unfortunately I was distracted by Other Stuff before I had time to break anything. 🙂

@itaiferber
Copy link
Contributor

itaiferber commented Mar 16, 2018

@belkadan That's what I'm saying — the type checker already has passes that validate conformances; we do that on the second pass:

// lib/Sema/TypeCheckDecl.cpp:4631
void visitClassDecl(ClassDecl *CD) {
  TC.checkDeclAttributesEarly(CD);
  TC.computeAccessLevel(CD);

  if (!IsSecondPass) {
    checkUnsupportedNestedType(CD);

    TC.validateDecl(CD);
    if (!CD->hasValidSignature())
      return;

    TC.requestSuperclassLayout(CD);
    TC.DeclsToFinalize.remove(CD);

    {
      // Check for circular inheritance.
      SmallVector<ClassDecl *, 8> path;
      path.push_back(CD);
      checkCircularity(TC, CD, diag::circular_class_inheritance,
                       diag::class_here, path);
    }
  }

  // If this class needs an implicit constructor, add it.
  if (!IsFirstPass)
    TC.addImplicitConstructors(CD);

  CD->addImplicitDestructor();

  if (!IsFirstPass && !CD->isInvalid())
    TC.checkConformancesInContext(CD, CD);

  for (Decl *Member : CD->getMembers())
    visit(Member);

  // <snip>

  TC.checkDeclAttributes(CD);
}

Or am I still misunderstanding?

(I'd try to step through here but unfortunately my machine can't seem to build Swift at the moment)

@lorentey
Copy link
Member

lorentey commented Mar 16, 2018

@itaiferber I believe external decls aren't fully type checked.

@itaiferber
Copy link
Contributor

itaiferber commented Mar 16, 2018

@lorentey Ah; that might do it. If the class is defined in a separate file it's considered "external" and thus might not be fully validated?

@belkadan
Copy link
Contributor

belkadan commented Mar 16, 2018

Right, anything that has to do with a "pass" only applies to declarations in the current file. validateDecl doesn't normally go through that.

(…except for some types where it does, because past Jordan never finished factoring it out.)

@lorentey: That's about declarations imported from Clang, not declarations in other files.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Mar 16, 2018

Comment by Ammo Goettsch (JIRA)

As an aside: There could be other cases though where this is not happening. Shouldn't the compiler create an error instead of emitting code that ends up accessing effectively vtable[-1] ? It would be nice if the compiler could catch future instances of these incomplete types before you execute the code in the field...

@belkadan
Copy link
Contributor

belkadan commented Mar 16, 2018

The compiler can't know that something that doesn't exist is supposed to exist. If it did know that, it could just do the creating itself.

(It's most likely not doing vtable[-1]; it'd be doing vtable[2] when it should be doing vtable[3].)

@swift-ci
Copy link
Collaborator Author

swift-ci commented Mar 16, 2018

Comment by Ammo Goettsch (JIRA)

Read my original debugging in this bug report. It is fetching the vtable size, instead of one of the entries. So either some index is initialized to -1, or the vtable stores its size in slot 0 (didn't check the compiler code.) If this was a normal vtable error where it is getting the wrong function because it disagrees on layout with another module, then you would be correct. But this is an out of bounds access. I even messed with the classes I used for testing to try different vtable sizes to confirm it is always accessing one word to the "left" of the vtable entries and getting the table size.

Just to reiterate: then it jumps to the table size interpreted as an address. The correct function it SHOULD have called is in fact in the vtable, as the first entry stored one word to the right of that. So it isn't missing the function either.

@slavapestov
Copy link
Member

slavapestov commented Apr 20, 2018

#16057

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants