Lucy
  1. Lucy
  2. LUCY-89

Tighten Clownfish method invocation type checking

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Clownfish, Core, Perl bindings
    • Labels:
      None

      Description

      Clownfish method invocation is implemented using C static inline functions.
      The first argument to these functions is the object itself, which up till now
      has been cast to "const void*". It is better to have the first argument match
      the type of the invoker, so that the C compiler will issue warnings if you try
      to invoke a method on an inappropriate object, e.g. "PriQ_Pop(hash)".

      1. tighten_types.diff
        4 kB
        Marvin Humphrey
      2. fallout.diff
        52 kB
        Marvin Humphrey

        Activity

        Hide
        Marvin Humphrey added a comment -

        The first patch, "tighten_types.diff", changes the Clownfish code generator to
        avoid the cast to "const void*".

        Before:

        static CHY_INLINE lucy_Obj*
        Lucy_PriQ_Pop(const void *vself)
        {
            lucy_PriorityQueue *const self = (lucy_PriorityQueue*)vself;
            char *const method_address = *(char**)self + Lucy_PriQ_Pop_OFFSET;
            const lucy_PriQ_pop_t method = *((lucy_PriQ_pop_t*)method_address);
            return method(self);
        }
        

        After:

        static CHY_INLINE lucy_Obj*
        Lucy_PriQ_Pop(const lucy_PriorityQueue *self)
        {
            char *const method_address = *(char**)self + Lucy_PriQ_Pop_OFFSET;
            const lucy_PriQ_pop_t method = *((lucy_PriQ_pop_t*)method_address);
            return method((lucy_PriorityQueue*)self);
        }
        

        The "const" cast has been left in place, even though it is wrong quite a lot
        – whenever a method modifies the object. This is mildly dangerous, because
        you can e.g. invoke CB_Cat_Str(charbuf, "foo", 3) on an constant CharBuf and
        the compiler will not complain. However, in order to fix that problem, we
        would need to audit all methods to determine which are safe to call on "const"
        objects and modify their method signatures – a cure which is worse than the
        disease.

        Show
        Marvin Humphrey added a comment - The first patch, "tighten_types.diff", changes the Clownfish code generator to avoid the cast to "const void*". Before: static CHY_INLINE lucy_Obj* Lucy_PriQ_Pop(const void *vself) { lucy_PriorityQueue *const self = (lucy_PriorityQueue*)vself; char *const method_address = *(char**)self + Lucy_PriQ_Pop_OFFSET; const lucy_PriQ_pop_t method = *((lucy_PriQ_pop_t*)method_address); return method(self); } After: static CHY_INLINE lucy_Obj* Lucy_PriQ_Pop(const lucy_PriorityQueue *self) { char *const method_address = *(char**)self + Lucy_PriQ_Pop_OFFSET; const lucy_PriQ_pop_t method = *((lucy_PriQ_pop_t*)method_address); return method((lucy_PriorityQueue*)self); } The "const" cast has been left in place, even though it is wrong quite a lot – whenever a method modifies the object. This is mildly dangerous, because you can e.g. invoke CB_Cat_Str(charbuf, "foo", 3) on an constant CharBuf and the compiler will not complain. However, in order to fix that problem, we would need to audit all methods to determine which are safe to call on "const" objects and modify their method signatures – a cure which is worse than the disease.
        Hide
        Marvin Humphrey added a comment -

        The second patch, "fallout.diff", deals with the fallout from the first patch,
        addressing all the new compiler warnings.

        Since the C compiler can't tell that e.g. Hash inherits from Obj,
        modifications such as these are necessary:

        -    Obj_Dec_RefCount(hash);
        +    Hash_Dec_RefCount(hash);
        

        I only found one instance of an inappropriate method invocation in the Lucy
        code base, where a Hash method was being invoked on a ZombieCharBuf in a test
        file:

        -    ASSERT_TRUE(batch, Hash_Equals(&foo, Hash_Fetch(hash, (Obj*)&forty)),
        +    ASSERT_TRUE(batch, ZCB_Equals(&foo, Hash_Fetch(hash, (Obj*)&forty)),
        

        This turns out to be harmless because both Hash and ZombieCharBuf inherit the
        Equals() method from Obj, so it resides at the same offset in both VTables.

        However, I think it's inevitable that without the improved type checking,
        future contributors to Lucy would have have been bitten at some point. Since
        function pointers with completely different signatures may reside at the same
        offset in different VTables, the result would be severe: most often, a memory
        error.

        Show
        Marvin Humphrey added a comment - The second patch, "fallout.diff", deals with the fallout from the first patch, addressing all the new compiler warnings. Since the C compiler can't tell that e.g. Hash inherits from Obj, modifications such as these are necessary: - Obj_Dec_RefCount(hash); + Hash_Dec_RefCount(hash); I only found one instance of an inappropriate method invocation in the Lucy code base, where a Hash method was being invoked on a ZombieCharBuf in a test file: - ASSERT_TRUE(batch, Hash_Equals(&foo, Hash_Fetch(hash, (Obj*)&forty)), + ASSERT_TRUE(batch, ZCB_Equals(&foo, Hash_Fetch(hash, (Obj*)&forty)), This turns out to be harmless because both Hash and ZombieCharBuf inherit the Equals() method from Obj, so it resides at the same offset in both VTables. However, I think it's inevitable that without the improved type checking, future contributors to Lucy would have have been bitten at some point. Since function pointers with completely different signatures may reside at the same offset in different VTables, the result would be severe: most often, a memory error.
        Hide
        Marvin Humphrey added a comment -

        Committed as r894043 and r894044.

        Show
        Marvin Humphrey added a comment - Committed as r894043 and r894044.

          People

          • Assignee:
            Marvin Humphrey
            Reporter:
            Marvin Humphrey
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development