[PATCH][libgfortran] Fix uninitialized variable use in fallback_access

[PATCH][libgfortran] Fix uninitialized variable use in fallback_access

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Hi all,

I've been tracking down a bug in a Fortran program on a newlib target and it boils down to fallback_access doing something bad.
The unconditional calls to close cause havoc when open doesn't get called due to the short-circuiting in the if-statement above
because the fd is uninitialised. In my environment GCC ends up calling close on file descriptor 0, thus trying to close stdin.

This patch tightens up the calling so that close is called only when the corresponding open call succeeded.
With this my runtime failure disappears.

Bootstrapped and tested on aarch64-none-linux-gnu.
Though that doesn't exercise this call I hope it's an obviously correct change.

Ok for trunk and the branches?

Thanks,
Kyrill

2018-09-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * io/unix.c (fallback_access): Avoid calling close on
     uninitialized file descriptor.

[-- Attachment #2: fort-access.patch --]
[-- Type: text/x-patch, Size: 740 bytes --]

diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 61e9f7997b25819514af546b50aa1d00b1d116f9..8081d6f96b2f6cd2489e876c8921cfb3510287ca 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -149,13 +149,21 @@ fallback_access (const char *path, int mode)
 {
   int fd;
 
-  if ((mode & R_OK) && (fd = open (path, O_RDONLY)) < 0)
-    return -1;
-  close (fd);
+  if (mode & R_OK)
+    {
+      if ((fd = open (path, O_RDONLY)) < 0)
+	return -1;
+      else
+	close (fd);
+    }
 
-  if ((mode & W_OK) && (fd = open (path, O_WRONLY)) < 0)
-    return -1;
-  close (fd);
+  if (mode & W_OK)
+    {
+      if ((fd = open (path, O_WRONLY)) < 0)
+	return -1;
+      else
+	close (fd);
+    }
 
   if (mode == F_OK)
     {

Re: [PATCH][libgfortran] Fix uninitialized variable use in fallback_access

[Paul Richard Thomas]

Hi Kyrill,

This indeed is an obvious change. OK for trunk and 7- and 8-branches.

Thanks for the patch

Paul

On 14 September 2018 at 09:06, Kyrill  Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi all,
>
> I've been tracking down a bug in a Fortran program on a newlib target and it
> boils down to fallback_access doing something bad.
> The unconditional calls to close cause havoc when open doesn't get called
> due to the short-circuiting in the if-statement above
> because the fd is uninitialised. In my environment GCC ends up calling close
> on file descriptor 0, thus trying to close stdin.
>
> This patch tightens up the calling so that close is called only when the
> corresponding open call succeeded.
> With this my runtime failure disappears.
>
> Bootstrapped and tested on aarch64-none-linux-gnu.
> Though that doesn't exercise this call I hope it's an obviously correct
> change.
>
> Ok for trunk and the branches?
>
> Thanks,
> Kyrill
>
> 2018-09-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     * io/unix.c (fallback_access): Avoid calling close on
>     uninitialized file descriptor.



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Teaching and debugging OpenMP in gfortran (or otherwise!)

[N.M. Maclaren]

Sorry about this, but searching in the usual places has found nothing
useful, and I am interested in gfortran's direction.

The kiddies do not like my OpenMP course, because it is all about what not
to do, but the problem is that even egregious data races do not show up in
test codes and there doesn't seem to be a usable testing tool. The best for
use with gfortran and gcc seems to be valgrind (helgrind or drd), but both
produce a hopeless number of false positives even in gcc 4.9.4 rebuilt with
no futexes and valgrind 3.13.  Interestingly, gfortran is worse than gcc,
and my examples are functionally identical (I teach both languages at once).

This isn't just a teaching problem, as OpenMP experts will know.  It's
horribly common for programs to fail, sometimes, after long running times,
on some systems - usually giving wrong answers with no diagnostic :-(

Has anyone any comments on this - except "you're stuffed" :-)  I know the
difficulty involved, so am not expecting a good solution, nor thinking
about doing it, but is there even a partial solution to this for gfortran?


Regards,
Nick Maclaren.

Re: Teaching and debugging OpenMP in gfortran (or otherwise!)

[Paul Richard Thomas]

Hi Nick,

One remark that I have is that your gfortran is rather out of date. A
lot of bugs that show up with valgrind have been fixed since then. The
second thought is that you might contact Jakub Jelnik for questions
concerning OpenMP debugging.

Cheers

Paul


On 15 September 2018 at 13:02, N.M. Maclaren <nmm1@cam.ac.uk> wrote:
> Sorry about this, but searching in the usual places has found nothing
> useful, and I am interested in gfortran's direction.
>
> The kiddies do not like my OpenMP course, because it is all about what not
> to do, but the problem is that even egregious data races do not show up in
> test codes and there doesn't seem to be a usable testing tool. The best for
> use with gfortran and gcc seems to be valgrind (helgrind or drd), but both
> produce a hopeless number of false positives even in gcc 4.9.4 rebuilt with
> no futexes and valgrind 3.13.  Interestingly, gfortran is worse than gcc,
> and my examples are functionally identical (I teach both languages at once).
>
> This isn't just a teaching problem, as OpenMP experts will know.  It's
> horribly common for programs to fail, sometimes, after long running times,
> on some systems - usually giving wrong answers with no diagnostic :-(
>
> Has anyone any comments on this - except "you're stuffed" :-)  I know the
> difficulty involved, so am not expecting a good solution, nor thinking
> about doing it, but is there even a partial solution to this for gfortran?
>
>
> Regards,
> Nick Maclaren.
>
>



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein

Re: Teaching and debugging OpenMP in gfortran (or otherwise!)

[N.M. Maclaren]

On Sep 15 2018, Paul Richard Thomas wrote:
>
>One remark that I have is that your gfortran is rather out of date. A
>lot of bugs that show up with valgrind have been fixed since then. The
>second thought is that you might contact Jakub Jelnik for questions
>concerning OpenMP debugging.

Thanks very much.  I think that I am stuffed :-(

That's not the version I use, but one that was close to the version
referred to in the valgrind 3.13 release notes.  I am trying to rebuild
4.3 (yes, really), which is the actual one referred to.  I could certainly
rebuild the latest and greatest gfortran, but am not optimistic, if the
last version valgrind tested with was 4.3 :-(

I will try contacting him.  I am specifically asking about tools for
automatic checking for race conditions - most of my course is about
writing OpenMP that actually works, and it goes into great length about
how hard it is to debug.

Regards,
Nick Maclaren.

Re: Teaching and debugging OpenMP in gfortran (or otherwise!)

[Thomas Koenig]

Am 15.09.2018 um 17:52 schrieb N.M. Maclaren:
> I am specifically asking about tools for
> automatic checking for race conditions - most of my course is about
> writing OpenMP that actually works, and it goes into great length about
> how hard it is to debug.

I have not debugged OpenMP myself, but for pthread - based code (the
async I/O stuff) valgrind has been invaluable. It helped Nicolas and
me to debug a one in 20000 occurrence of a deadlock...

Specifically, try to use

valgrind --tool=helgrind

and

valgrind --tool=drd

and watch the bugs come crawling out :-)

Regards

	Thomas

Re: Teaching and debugging OpenMP in gfortran (or otherwise!)

[N.M. Maclaren]

On Sep 15 2018, Paul Richard Thomas wrote:
>
>One remark that I have is that your gfortran is rather out of date. A
>lot of bugs that show up with valgrind have been fixed since then.

No better in 8.2.  4.3 won't build, and I don't have time to work out how
to bypass the problems.  Unless I have made a stupid error, it's hopeless.

Regards,
Nick Maclaren.