wrong usage of the `malloc' function attribute

Bug #1123588 reported by Matthias Klose
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
gcc
Invalid
Medium
libnih
In Progress
Undecided
James Hunt
upstart
Fix Released
High
James Hunt
gcc-4.7 (Ubuntu)
Fix Released
Undecided
Unassigned
Raring
Fix Released
Undecided
Unassigned
libnih (Ubuntu)
Fix Released
Undecided
James Hunt
Raring
Fix Released
Undecided
James Hunt
upstart (Ubuntu)
Fix Released
High
James Hunt
Raring
Fix Released
High
James Hunt

Bug Description

seen with -21ubuntu1, not with -20ubuntu1, reverting the fix for PR53844 avoids the issue.

I think we need a reproducer in upstart, which doesn't just hang.

Tags: patch
Revision history for this message
In , Doko-v (doko-v) wrote :

works with 4.6 branch, 4.7.2 and the trunk. introduced in r195708 by the fix for PR53844.

I don't have a reduced testcase yet. the unreduced test case is that the upstart build in Ubuntu raring hangs in the testsuite with:

  Testing job_deserialise() ptrace handling
  BAD: wrong value for job->goal, expected 1 got 0
   at tests/test_job.c:7331 (deserialise_ptrace_next).

still trying to isolate the failure.

afaics, it is not architecture specific.

Even if backports are tested together, it would help if independent patches could be committed separately to the branches.

Changed in gcc:
importance: Unknown → Medium
status: Unknown → New
Revision history for this message
Launchpad Janitor (janitor) wrote : Re: [4.7 Regression] wrong code with the fix for PR53844

This bug was fixed in the package gcc-4.7 - 4.7.2-21ubuntu2

---------------
gcc-4.7 (4.7.2-21ubuntu2) raring; urgency=low

  * Update to SVN 20130212 (r195985) from the gcc-4_7-branch.
    - Fix PR rtl-optimization/56275, PR target/56043, PR c++/56268,
      PR c++/56247, PR target/52122.
  * Revert the fix for PR optimization/53844. LP: #1123588.
 -- Matthias Klose <email address hidden> Wed, 13 Feb 2013 01:16:12 +0100

Changed in gcc-4.7 (Ubuntu):
status: New → Fix Released
Matthias Klose (doko)
Changed in gcc-4.7 (Ubuntu):
status: Fix Released → Confirmed
Revision history for this message
Matthias Klose (doko) wrote :

attaching a reduced test_job.[ci], and the object files to build the executable. Seen when upstart is built with -O[s123], works with -O0.

forcing no optimization just on test_deserialise_ptrace is enough to get the code working again:

  void test_deserialise_ptrace (void) __attribute__ ((optimize ("O0")));

if you debug the issue, make sure to unpack (or install) the cpp-4.7 and gcc-4.7 packages in version 4.7.2-21ubuntu1.

Checking -O0 + -f options enabled in -O1, I couldn't identify an option which triggers the issue.

Changed in gcc:
status: New → Incomplete
Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in upstart (Ubuntu):
status: New → Confirmed
Revision history for this message
In , Doko-v (doko-v) wrote :

Created attachment 29482
test case

no reduced test case yet, but there is a diff in the tree dump with -fdump-tree-dse in the test_deserialise_ptrace function:

diff -u <working version> <not-working version>

--- revert/upstart-1.6.1/init/test_job.c.132t.dse2
+++ norevert/upstart-1.6.1/init/test_job.c.132t.dse2
@@ -367,12 +367,8 @@
   abort ();

 <bb 44>:
- job_64->goal = 1;
- job_64->state = 3;
   D.9269_65 = job_64->pid;
   *D.9269_65 = pid_49;
- job_64->trace_forks = 0;
- job_64->trace_state = 3;

 <bb 45>:
   __ret_66 = nih_str_array_new (0B);

Revision history for this message
In , Doko-v (doko-v) wrote :

with -fdump-tree-dse-details:

--- revert/upstart-1.6.1/init/test_job.c.132t.dse2
+++ norevert/upstart-1.6.1/init/test_job.c.132t.dse2
@@ -35,6 +35,14 @@

 ;; Function test_deserialise_ptrace (test_deserialise_ptrace, funcdef_no=60, decl_uid=9022, cgraph_uid=60)

+ Deleted dead store 'job_64->trace_state = 3;
+'
+ Deleted dead store 'job_64->trace_forks = 0;
+'
+ Deleted dead store 'job_64->state = 3;
+'
+ Deleted dead store 'job_64->goal = 1;
+'
 test_deserialise_ptrace ()
 {
   long unsigned int D.9415;
@@ -367,12 +375,8 @@
   abort ();

 <bb 44>:
- job_64->goal = 1;
- job_64->state = 3;
   D.9269_65 = job_64->pid;
   *D.9269_65 = pid_49;
- job_64->trace_forks = 0;
- job_64->trace_state = 3;

 <bb 45>:
   __ret_66 = nih_str_array_new (0B);

Revision history for this message
In , Rguenth (rguenth) wrote :

I don't see how this is a bug.

  job_64 = job_new (class_39, "");
  # DEBUG job => job_64
  if (job_64 == 0B)
    goto <bb 43>;
  else
    goto <bb 44>;

<bb 43>:
  # DEBUG __fmt => "BAD: wrong value for %s, got unexpected %p\n\tat %s:%d (%s).\n"
  __printf_chk (1, "BAD: wrong value for %s, got unexpected %p\n\tat %s:%d (%s).\n", "job", 0B, "tests/test_job.c", 111, &__FUNCTION__);
  abort ();

<bb 44>:
  D.9269_65 = job_64->pid;
  *D.9269_65 = pid_49;

that was the last use of the memory pointed to by job_64 (job_new is marked as malloc). In fact, job_64->pid is uninitialized! To quote the documentation
of the malloc attribute:

@item malloc
@cindex @code{malloc} attribute
The @code{malloc} attribute is used to tell the compiler that a function
may be treated as if any non-@code{NULL} pointer it returns cannot
alias any other pointer valid when the function returns and that the memory
has undefined content.
This will often improve optimization.
Standard functions with this property include @code{malloc} and
@code{calloc}. @code{realloc}-like functions do not have this
property as the memory pointed to does not have undefined content.

it appears that the returned memory does not have undefined contents.

Revision history for this message
In , Rguenth (rguenth) wrote :

Thus, invalid.

Revision history for this message
Matthias Klose (doko) wrote :

according to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301#c3 this is not an issue with GCC, but job_new shouldn't be marked with the malloc attribute. The following patch lets the test case pass without the GCC change reverted. Maybe other usages of the malloc attribute should be revisited.

 --- init/job.h.orig 2012-12-04 17:14:47.000000000 +0000
+++ init/job.h 2013-02-18 13:06:31.792691230 +0000
@@ -165,7 +165,7 @@
 NIH_BEGIN_EXTERN

 Job * job_new (JobClass *class, const char *name)
- __attribute__ ((warn_unused_result, malloc));
+ __attribute__ ((warn_unused_result));
 void job_register (Job *job, DBusConnection *conn, int signal);

 void job_change_goal (Job *job, JobGoal goal);

Changed in upstart (Ubuntu):
status: Confirmed → Triaged
importance: Undecided → High
assignee: nobody → James Hunt (jamesodhunt)
Changed in gcc-4.7 (Ubuntu):
status: Confirmed → Invalid
Matthias Klose (doko)
Changed in upstart (Ubuntu Raring):
milestone: none → ubuntu-13.04-beta-1
Revision history for this message
In , Jakub-gcc (jakub-gcc) wrote :

Well, perhaps we need to improve documentation, because for calloc the memory doesn't have undefined contents either, it is well defined to be all zeros.

Revision history for this message
In , Rguenther-suse (rguenther-suse) wrote :

On Mon, 18 Feb 2013, jakub at gcc dot gnu.org wrote:

>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56301
>
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> CC| |jakub at gcc dot gnu.org
>
> --- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-02-18 13:31:28 UTC ---
> Well, perhaps we need to improve documentation, because for calloc the memory
> doesn't have undefined contents either, it is well defined to be all zeros.

Well, it points to nothing ;) The bug here is that probably
job_new links the allocated memory into some global list or so,
so it's not about initializing the memory but the fact that it
_is_ aliased by other things.

Yes, we can probably give a few examples of what is not appropriate
use of 'malloc'.

Do you think I should revert the patch on the branch nevertheless?
(it was a fix for a missed-optimization regression only ...)

Revision history for this message
In , Jakub-gcc (jakub-gcc) wrote :

(In reply to comment #6)
> Do you think I should revert the patch on the branch nevertheless?
> (it was a fix for a missed-optimization regression only ...)

Yeah, missed-optimization regression can wait for 4.8, but just the tree-ssa-dse.c part + related testcase, not all the other fixes.

Changed in gcc:
status: Incomplete → Invalid
Revision history for this message
Matthias Klose (doko) wrote :

I first attempt to remove the malloc function attribute in places where it doesn't belong.

summary: - [4.7 Regression] wrong code with the fix for PR53844
+ wrong usage of the `malloc' function attribute
Revision history for this message
In , Doko-v (doko-v) wrote :

> so it's not about initializing the memory but the fact that it
> _is_ aliased by other things.

yes, the value returned was saved in a caching data structure.

tags: added: patch
Steve Langasek (vorlon)
Changed in upstart:
status: New → Triaged
importance: Undecided → High
assignee: nobody → James Hunt (jamesodhunt)
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package gcc-4.7 - 4.7.2-22ubuntu1

---------------
gcc-4.7 (4.7.2-22ubuntu1) raring; urgency=low

  * Merge with Debian; remaining changes:
    - Build from upstream source.

gcc-4.7 (4.7.2-22) experimental; urgency=low

  * Update to SVN 20130222 (r196236) from the gcc-4_7-branch.
    - Fix PR rtl-optimization/56275, PR target/56043, PR c++/56268,
      PR c++/56247, PR target/52122, PR c++/56291, PR target/52123,
      PR target/52122, PR c++/54276, PR c++/52026, PR c++/55710, PR c++/56135,
      PR fortran/53537, PR middle-end/56217, PR libstdc++/55043,
      PR other/56245, PR bootstrap/56258, PR tree-optimization/56350,
      PR tree-optimization/56381, PR tree-optimization/56250,
      PR middle-end/56217, PR tree-optimization/55110, PR c++/40405,
      PR c++/56395, PR c++/56241, PR c++/56239, PR c++/56237, PR ada/56271,
      PR fortran/56385, PR libfortran/30162.
  * Revert the fix for PR optimization/53844. LP: #1123588.
  * Update the Linaro support to the 4.7-2013.02 release.
 -- Matthias Klose <email address hidden> Sat, 23 Feb 2013 06:43:05 +0100

Changed in gcc-4.7 (Ubuntu Raring):
status: Invalid → Fix Released
Revision history for this message
James Hunt (jamesodhunt) wrote :

A common convention in Upstart+NIH is for functions to accept a parent pointer as in the following example:

Foo *make_foo (const void *parent) __attribute__ ((malloc));

Foo *
make_foo (const void *parent)
{
 Foo *foo = nih_new (parent, Foo);

        /* ... */

 return foo;
}

But, if I'm understanding this correctly (?), using the malloc attribute here is also invalid iff parent != NULL since nih_new() ultimately saves the value of foo to the parents context before returning the address of foo.

Revision history for this message
Matthias Klose (doko) wrote :

yes, that's my understanding.

Revision history for this message
James Hunt (jamesodhunt) wrote :

I've discussed this with Scott. He believes the description of the malloc attribute has changed since libnih first started using it. The current description does make its use in libnih and upstart invalid seemingly.

Revision history for this message
Matthias Klose (doko) wrote :

according to http://gcc.gnu.org/onlinedocs/ this was documented from 4.1 on, didn't check earlier releases.

James Hunt (jamesodhunt)
Changed in libnih:
assignee: nobody → James Hunt (jamesodhunt)
Changed in libnih (Ubuntu Raring):
assignee: nobody → James Hunt (jamesodhunt)
Revision history for this message
James Hunt (jamesodhunt) wrote :

Scott is right - the description of this attribute *has* changed. Compare the current description to:

http://web.archive.org/web/20060824111504/http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

Hence, either the attribute used to be incorrectly described and we were just lucky not to see any issues until now, or the meaning of this attribute has been changed by the gcc folk since it was introduced.

Revision history for this message
Matthias Klose (doko) wrote :

which already says ...
"The malloc attribute is used to tell the compiler that a function may be treated as if any non-NULL pointer it returns cannot alias any other pointer valid when the function returns."

James Hunt (jamesodhunt)
Changed in upstart:
status: Triaged → Fix Released
Changed in libnih:
status: New → In Progress
Changed in upstart (Ubuntu Raring):
status: Triaged → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package libnih - 1.0.3-4ubuntu16

---------------
libnih (1.0.3-4ubuntu16) raring; urgency=low

  * debian/{libnih1.postinst,libnih-dbus1.postinst}: Force an upgrade to
    restart Upstart (to pick up new package version) if the running
    instance supports it.
  * Merge of important fixes from lp:~upstart-devel/libnih/nih
    (LP: #776532, LP: #777097, LP: #834813, LP: #1123588).
 -- James Hunt <email address hidden> Thu, 14 Mar 2013 09:14:22 +0000

Changed in libnih (Ubuntu Raring):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.