cronolog doesn't update links due to stat() on NULL pathname

Bug #1770676 reported by Scott Emmons
14
This bug affects 2 people
Affects Status Importance Assigned to Milestone
gcc
Invalid
Medium
cronolog (Debian)
New
Unknown
cronolog (Ubuntu)
Confirmed
Undecided
Unassigned
gcc-5 (Ubuntu)
Invalid
Undecided
Unassigned
gcc-7 (Ubuntu)
Invalid
Undecided
Unassigned

Bug Description

While investigating why cronolog was no longer changing a log symlink in bionic, I discovered a bug in gcc (reproducable in 7.3.0-16ubuntu3 in bionic and 5.4.0-6ubuntu1~16.04.9 in xenial) for code compiled with -O2. I have created a simple testcase to reproduce the problem, which is attached.

You can reproduce the problem with the following:

# ====
set -x
echo "First, we compile and run without optimization:"
gcc -o testcase testcase.c && ./testcase

echo "Then, we compile and run with optimization:"
gcc -O2 -o testcase testcase.c && ./testcase
# ====

Which outputs:
+ echo 'First, we compile and run without optimization:'
First, we compile and run without optimization:
+ gcc -o testcase testcase.c
+ ./testcase
foo in func() is NULL (expected)
foo in func() is NULL (expected)
foo in main() is NULL (expected)
foo in main() is NULL (expected)
+ echo 'Then, we compile and run with optimization:'
Then, we compile and run with optimization:
+ gcc -O2 -o testcase testcase.c
+ ./testcase
foo in func() is NULL (expected)
foo in func() is not NULL (NOT EXPECTED!)
foo in main() is NULL (expected)
foo in main() is NULL (expected)

The problem occurs after calling:

  stat(foo, &stat_buf);

Where foo is a NULL pointer. After the return from this function, foo will no longer be NULL when the code is compiled with optimization - but only when the pointer is a function parameter of func(). The issue does not occur when the same codepath is called in main(). (You can argue calling stat with a NULL pointer is bad behavior, however this code has been in cronolog, working fine when compiled with -O2, for years).

I can reproduce this easily on multiple systems, bionic with "gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0" and xenial with "gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609".

I could not initially reproduce the cronolog testcase in xenial, leading me to some suspicion that it may be related to retpoline or other recent compiler changes - and in fact after recompiling cronolog on xenial I can reproduce the issue there as well.

Revision history for this message
Scott Emmons (lscotte) wrote :
Scott Emmons (lscotte)
description: updated
description: updated
Revision history for this message
Scott Emmons (lscotte) wrote :

To reproduce this in cronolog:

echo hello | cronolog --symlink=/tmp/foo /tmp/foo.%Y%m%d-%H%M%S.log

This will create a new log every second (effectively each time the command is run). The correct behavior is that the /tmp/foo symlink should move to the new log file. In the broken case, the link does not move due to the if statement of "prevlinkname" returning true when it should be false [1]. If you test the value of "prevlinkname" for NULL before and after the stat(prevlinkname...) call at L250 (similar to my testcase.c), the pointer is no longer null after the call to stat().

[1] https://github.com/fordmason/cronolog/blob/master/src/cronoutils.c#L256

Revision history for this message
Launchpad Janitor (janitor) wrote :

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

Changed in gcc-5 (Ubuntu):
status: New → Confirmed
Changed in gcc-7 (Ubuntu):
status: New → Confirmed
Changed in gcc:
importance: Unknown → Medium
status: Unknown → New
Revision history for this message
Scott Emmons (lscotte) wrote :

It looks like GCC has determined this is expected to be undefined behavior, so I'll be getting a pull request into cronolog and then work with MOTU to get the patch in.

Matthias Klose (doko)
Changed in cronolog (Ubuntu):
status: New → Confirmed
Changed in gcc-5 (Ubuntu):
status: Confirmed → Invalid
Changed in gcc-7 (Ubuntu):
status: Confirmed → Invalid
Revision history for this message
Scott Emmons (lscotte) wrote :

Issue and pull request for cronolog submitted.

Issue: https://github.com/fordmason/cronolog/issues/4
Pull request: https://github.com/fordmason/cronolog/pull/5

Revision history for this message
Scott Emmons (lscotte) wrote :
Scott Emmons (lscotte)
summary: - gcc optimizer bug
+ cronolog doesn't update links due to stat() on NULL pathname
Changed in cronolog (Debian):
status: Unknown → New
Changed in gcc:
status: New → Invalid
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.