472570 - Invalid pointer handling in nsCOMPtr.h

Status: VERIFIED DUPLICATE of bug 351231

(Core :: XPCOM, defect)

Description

[Jacek Piskozub]

This is a spill-off from Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=468415

The Fedora 10 Seamonkey builds made with gcc-4.3.2-7 crash when biuld with -O2

The reason is (copying explicit from comment 34 of the Fedora bug by [e-mail]):

====start_of_bug_description====

Works with -O2 -fno-strict-aliasing for test.ii (nsCOMPtr.ii flags don't
matter).
This is obvious aliasing violation, so seamonkey gets what it deserves.

#if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 3)
  // Our use of nsCOMPtr_base::mRawPtr violates the C++ standard's aliasing
  // rules. Mark it with the may_alias attribute so that gcc 3.3 and higher
  // don't reorder instructions based on aliasing assumptions for
  // this variable.  Fortunately, gcc versions < 3.3 do not do any
  // optimizations that break nsCOMPtr.

  #define NS_MAY_ALIAS_PTR(t)    t*  __attribute__((__may_alias__))
#else
  #define NS_MAY_ALIAS_PTR(t)    t*
#endif

and it is used in
  NS_MAY_ALIAS_PTR(nsISupports) mRawPtr.
This is just great misunderstanding of what may_alias attribute is though.

`may_alias'
     Accesses through pointers to types with this attribute are not
     subject to type-based alias analysis, but are instead assumed to
     be able to alias any other type of objects.  In the context of
     6.5/7 an lvalue expression dereferencing such a pointer is treated
     like having a character type.  See `-fstrict-aliasing' for more
     information on aliasing issues.  This extension exists to support
     some vector APIs, in which pointers to one vector type are
     permitted to alias pointers to a different vector type.

     Note that an object of a type with this attribute does not have any
     special semantics.
Especially read the last note.  As strict aliasing violations aren't
reads/writes through the mRawPtr pointer, but reads/writes to the mRawPtr
variable (sometimes written/read as void *, sometimes as nsISupports *,
sometimes other pointers).  So, if you want to avoid strict aliasing violation,
you'd need:
     if (((__builtin_expect((!((rv) & 0x80000000)), 1)))) {
-      nsISupports *p_tmp = out.mRawPtr;
-      printf("p_out = %p\n",p_tmp);
-      nsIOutputStream *p_stream1 = (nsIOutputStream *)p_tmp;
+      nsISupports *__attribute__((may_alias)) *p_tmp = &out.mRawPtr;
+      printf("p_out = %p\n",*p_tmp);
+      nsIOutputStream *p_stream1 = (nsIOutputStream *)*p_tmp;
       printf("p_stream1 = %p\n",p_stream1);
   }
(and all accesses, at least those which the compiler can see, some are hidden
into begin_assignment) to mRawPtr would need to be done similarly).
And where you use begin_assignment or StartAssignment, you want to replace
all the method return values and all the casts NS_REINTERPRET_CAST, such that
they return {void,nsISupports,T} *__attribute__((__may_alias__)) {*,&,...}.

====end_of_bug_description====

More info on the Fedora bug page.

Comment 1

[Benjamin Smedberg]

We build with -fno-strict-aliasing by default. How are we ending up compiling this code with aliasing enabled?

Comment 2

[Kai Engert [:KaiE:]]

Adding [e-mail] as he last touched this, according to cvs blame.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #0)
> all the method return values and all the casts NS_REINTERPRET_CAST, such that

Given that you're referring to NS_REINTERPRET_CAST, which doesn't exist in Gecko 1.9, I'm wondering if you're using a Gecko version with an nsCOMPtr.h that doesn't include the fix to bug 351231.

Comment 4

[Jacek Piskozub]

David, this is a Seamonkey bug. So it is possible that I marked "Version:Trunk" in error. This should be "Version:1.8 Branch". Sorry.

Changing.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Mozilla binaries are okay. This bug affects Fedora seamonkey only because we build them w/o -fno-strict-aliasing flag and seamonkey doesn't contain fix from bug 351231.

Comment 6

[Benjamin Smedberg]

Then this is just a straight dup, and you apparently want to backport either bug 351231 or bug 413253.

Comment 7

[Jacek Piskozub]

Benjamin, closing a bug as a dup of another closed bug means killing it. Unless you mean I should reopen bug 351231. Is this your intention?

Comment 8

[Benjamin Smedberg]

No, bug 351231 is FIXED in our trunk codebase. If you want it to be backported to some older codebase, set the appropriate approval requests for the old branch you want to land the patch on.