[PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[Mark H Weaver]

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

Hello all,

David Pirotte has been experiencing a problem where (reload-module ...)
was failing to trigger a recompilation even after the source file has
been modified.  We did some debugging, and discovered that when
'compiled_is_fresh' is called during the failed reload, the
'stat_source' structure contains garbage.

My investigation revealed a flaw in 'search_path'.  Although it is not
mentioned in the header comment, it is clearly supposed to fill the
'stat_buf' whenever it is successful (scm_primitive_load_path assumes
that it will, anyway).

The problem is that there's a case when 'search_path' leaves the
'stat_buf' uninitialized.  If the provided 'filename' is an absolute
pathname, then it simply returns this pathname unchanged without
touching the 'stat_buf'.  This is bad, because 'scm_primitive_load_path'
proceeds to pass this uninitialized 'stat_buf' to 'compiled_is_fresh'.

I've attached a patch to (hopefully) fix this problem.  Unfortunately,
David tells me that this doesn't solve his reload issue, so unless he
made a mistake in testing it, I guess there's another bug still lurking,
or perhaps this fix is incorrect.

Reviews solicited.

    Thanks,
      Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Fix search_path to fill stat_buf when given an absolute pathname --]
[-- Type: text/x-patch, Size: 3145 bytes --]

From b3e8ae048d87ab28c90b1c1c635a5116ee6f080c Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Wed, 1 Feb 2012 16:35:32 -0500
Subject: [PATCH] Fix search_path to fill stat_buf when given an absolute
 pathname

* libguile/load.c (search_path): When the provided 'filename' is an
  absolute pathname, perform a 'stat' on that pathname to fill the
  'stat_buf'.  Previously, 'stat_buf' was left uninitialized in this
  case, even though 'scm_primitive_load_path' assumes that 'stat_buf'
  will be filled.  Update the header comment to explicitly specify that
  'stat_buf' will be filled.  Also 'goto end' in a few failure cases
  instead of replicating its code.
---
 libguile/load.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/libguile/load.c b/libguile/load.c
index 0076218..ae9e4bd 100644
--- a/libguile/load.c
+++ b/libguile/load.c
@@ -419,8 +419,9 @@ scm_c_string_has_an_ext (char *str, size_t len, SCM extensions)
 
 /* Search PATH for a directory containing a file named FILENAME.
    The file must be readable, and not a directory.
-   If we find one, return its full filename; otherwise, return #f.
+   If we find one, return its full pathname; otherwise, return #f.
    If FILENAME is absolute, return it unchanged.
+   We also fill *stat_buf corresponding to the returned pathname.
    If given, EXTENSIONS is a list of strings; for each directory 
    in PATH, we search for FILENAME concatenated with each EXTENSION.  */
 static SCM
@@ -445,7 +446,7 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
   filename_len = strlen (filename_chars);
   scm_dynwind_free (filename_chars);
 
-  /* If FILENAME is absolute, return it unchanged.  */
+  /* If FILENAME is absolute and is still valid, return it unchanged.  */
 #ifdef __MINGW32__
   if (((filename_len >= 1) && 
        (filename_chars[0] == '/' || filename_chars[0] == '\\')) ||
@@ -457,14 +458,13 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
   if (filename_len >= 1 && filename_chars[0] == '/')
 #endif
     {
-      SCM res = filename;
-      if (scm_is_true (require_exts) &&
-          !scm_c_string_has_an_ext (filename_chars, filename_len,
+      if ((scm_is_false (require_exts) ||
+           scm_c_string_has_an_ext (filename_chars, filename_len,
                                     extensions))
-        res = SCM_BOOL_F;
-
-      scm_dynwind_end ();
-      return res;
+          && stat (filename_chars, stat_buf) == 0
+          && !(stat_buf->st_mode & S_IFDIR))
+        result = filename;
+      goto end;
     }
 
   /* If FILENAME has an extension, don't try to add EXTENSIONS to it.  */
@@ -483,8 +483,7 @@ search_path (SCM path, SCM filename, SCM extensions, SCM require_exts,
               {
                 /* This filename has an extension, but not one of the right
                    ones... */
-                scm_dynwind_end ();
-                return SCM_BOOL_F;
+                goto end;
               }
 	    /* This filename already has an extension, so cancel the
                list of extensions.  */
-- 
1.7.5.4

Re: [PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[Andy Wingo]

On Wed 01 Feb 2012 22:47, Mark H Weaver <mhw@netris.org> writes:

> The problem is that there's a case when 'search_path' leaves the
> 'stat_buf' uninitialized.  If the provided 'filename' is an absolute
> pathname, then it simply returns this pathname unchanged without
> touching the 'stat_buf'.  This is bad, because 'scm_primitive_load_path'
> proceeds to pass this uninitialized 'stat_buf' to 'compiled_is_fresh'.

Wow.  What a hairy function.  Thanks again for really diving in here.
The patch looks fine to me; clearly stat_buf should be filled in that
case.  Please push!

I (for one) am very happy to comment on patches :)  If at any time you
want comments from folks, please do continue to mail patches to the
list.  But, for patches that you consider to be obviously correct,
please feel free to use your judgment as to whether to push directly or
not.  Anyway, as you like :-)

Cheers,

Andy
-- 
http://wingolog.org/

Re: [PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[rixed]

> David Pirotte has been experiencing a problem where (reload-module ...)
> was failing to trigger a recompilation even after the source file has
> been modified.

Don't know if this is related or not, but it occured to me or my
coworkers several times that a given scm file was not recompiled to a new
.go file despite being changed, and that we had to manualy rm -rf the
.go cache. I somewhat get used to it since the fix is so easy, and never
tried  to understand nor to report it. :-(

Re: [PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[Mark H Weaver]

Replying to myself...

> David Pirotte has been experiencing a problem where (reload-module ...)
> was failing to trigger a recompilation even after the source file has
> been modified.  [...]
[...]
> I've attached a patch to (hopefully) fix this problem.  Unfortunately,
> David tells me that this doesn't solve his reload issue, so unless he
> made a mistake in testing it, [...]

It turned out that David made a mistake in testing, and this patch
_does_ fix his problem.  I pushed it to stable-2.0.  Thanks again to
David for reporting the problem and helping me track it down! :)

     Mark

Re: [PATCH] Fix search_path to fill stat_buf when given an absolute pathname

[Ian Price]

Mark H Weaver <mhw@netris.org> writes:

>> I've attached a patch to (hopefully) fix this problem.  Unfortunately,
>> David tells me that this doesn't solve his reload issue, so unless he
>> made a mistake in testing it, [...]
>
> It turned out that David made a mistake in testing, and this patch
> _does_ fix his problem.  I pushed it to stable-2.0.  Thanks again to
> David for reporting the problem and helping me track it down! :)

Cool, I've been having this issue somewhat intermittently, and it's good
to see a fix. I'll keep my eyes peeled for any recurrences.

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"