[PATCH v2 2/3] [SV 54161] Fix second expansion of $*

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Jouke Witteveen
Make sure the second expansion of $* in the prerequisites matches that of
$* in the recipe.  This means that if we would add the dir of the stem to
the prerequisites, we should replace % by $(*F) in the first expansion.
Otherwise, we replace it to $*, but make sure that the value of $* used
for expanding prerequisites matches that of $* in the recipe.
---
 src/implicit.c | 56 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/implicit.c b/src/implicit.c
index 7bdc8ba..e400dc6 100644
--- a/src/implicit.c
+++ b/src/implicit.c
@@ -220,8 +220,9 @@ pattern_search (struct file *file, int archive,
   struct patdeps *deplist = xmalloc (max_deps * sizeof (struct patdeps));
   struct patdeps *pat = deplist;
 
-  /* Names of possible dependencies are constructed in this buffer.  */
-  char *depname = alloca (namelen + max_pattern_dep_length);
+  /* Names of possible dependencies are constructed in this buffer.
+     We may replace % by $(*F) for second expansion, increasing the length.  */
+  char *depname = alloca (namelen + max_pattern_dep_length + 4);
 
   /* The start and length of the stem of FILENAME for the current rule.  */
   const char *stem = 0;
@@ -479,9 +480,10 @@ pattern_search (struct file *file, int archive,
                 }
             }
 
-          if (stemlen > GET_PATH_MAX)
+          if (stemlen + (check_lastslash ? pathlen : 0) > GET_PATH_MAX)
             {
-              DBS (DB_IMPLICIT, (_("Stem too long: '%.*s'.\n"),
+              DBS (DB_IMPLICIT, (_("Stem too long: '%s%.*s'.\n"),
+                                 check_lastslash ? pathdir : "",
                                  (int) stemlen, stem));
               continue;
             }
@@ -489,8 +491,19 @@ pattern_search (struct file *file, int archive,
           DBS (DB_IMPLICIT, (_("Trying pattern rule with stem '%.*s'.\n"),
                              (int) stemlen, stem));
 
-          strncpy (stem_str, stem, stemlen);
-          stem_str[stemlen] = '\0';
+          if (!check_lastslash)
+            {
+              memcpy (stem_str, stem, stemlen);
+              stem_str[stemlen] = '\0';
+            }
+          else
+            {
+              /* We want to prepend the directory from
+                 the original FILENAME onto the stem.  */
+              memcpy (stem_str, filename, pathlen);
+              memcpy (stem_str + pathlen, stem, stemlen);
+              stem_str[pathlen + stemlen] = '\0';
+            }
 
           /* If there are no prerequisites, then this rule matches.  */
           if (rule->deps == 0)
@@ -541,7 +554,7 @@ pattern_search (struct file *file, int archive,
                         }
                       memcpy (o, nptr, p - nptr);
                       o += p - nptr;
-                      memcpy (o, stem_str, stemlen);
+                      memcpy (o, stem, stemlen);
                       o += stemlen;
                       strcpy (o, p + 1);
                     }
@@ -592,10 +605,10 @@ pattern_search (struct file *file, int archive,
                      again.  This is not good if you have certain characters
                      in your stem (like $).
 
-                     Instead, we will replace % with $* and allow the second
-                     expansion to take care of it for us.  This way (since $*
-                     is a simple variable) there won't be additional
-                     re-expansion of the stem.  */
+                     Instead, we will replace % with $* or $(*F) and allow the
+                     second expansion to take care of it for us.  This way
+                     (since $* and $(*F) are simple variables) there won't be
+                     additional re-expansion of the stem.  */
 
                   p = lindex (nptr, nptr + len, '%');
                   if (p == 0)
@@ -606,13 +619,22 @@ pattern_search (struct file *file, int archive,
                   else
                     {
                       size_t i = p - nptr;
-                      memcpy (depname, nptr, i);
-                      memcpy (depname + i, "$*", 2);
-                      memcpy (depname + i + 2, p + 1, len - i - 1);
-                      depname[len + 2 - 1] = '\0';
-
+                      char *o = depname;
+                      memcpy (o, nptr, i);
+                      o += i;
                       if (check_lastslash)
-                        add_dir = 1;
+                        {
+                          add_dir = 1;
+                          memcpy (o, "$(*F)", 5);
+                          o += 5;
+                        }
+                      else
+                        {
+                          memcpy (o, "$*", 2);
+                          o += 2;
+                        }
+                      memcpy (o, p + 1, len - i - 1);
+                      o[len - i - 1] = '\0';
                     }
 
                   /* Set up for the next word.  */
--
2.23.0


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Jouke Witteveen
On Sat, Oct 26, 2019 at 12:24 PM Jouke Witteveen <[hidden email]> wrote:
>
> Make sure the second expansion of $* in the prerequisites matches that of
> $* in the recipe.  This means that if we would add the dir of the stem to
> the prerequisites, we should replace % by $(*F) in the first expansion.
> Otherwise, we replace it to $*, but make sure that the value of $* used
> for expanding prerequisites matches that of $* in the recipe.
> ---

Is there anything I can do to further review and/or acceptance of this
patch I sent last month?

Regards,
- Jouke

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Paul Smith-20
On Sat, 2019-11-23 at 12:22 +0100, Jouke Witteveen wrote:
> Is there anything I can do to further review and/or acceptance of this
> patch I sent last month?

I applied your changes although updates were needed.

For the future, please note that (a) changes should contain updates to the
regression tests to show the error and ensure the fix works, (b) commit
messages need to be formatted properly so that they can be generated into
ChangeLog entries, (c) we cannot use GNU libc-only functions like memrchr()
since GNU make is highly portable and runs on Windows, VMS, etc., not to
mention BSD, MacOS, and other POSIX systems where glibc is not available.

I added a regression test based on the Savannah bug report, reformatted the
commit messages, and added an implementation of memrchr() for systems where
it doesn't exist.

Thanks for the fix!


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Jouke Witteveen
On Wed, Dec 18, 2019 at 3:11 PM Paul Smith <[hidden email]> wrote:

> I applied your changes although updates were needed.
>
> For the future, please note that (a) changes should contain updates to the
> regression tests to show the error and ensure the fix works, (b) commit
> messages need to be formatted properly so that they can be generated into
> ChangeLog entries, (c) we cannot use GNU libc-only functions like memrchr()
> since GNU make is highly portable and runs on Windows, VMS, etc., not to
> mention BSD, MacOS, and other POSIX systems where glibc is not available.
>
> I added a regression test based on the Savannah bug report, reformatted the
> commit messages, and added an implementation of memrchr() for systems where
> it doesn't exist.

Thanks for fixing my commit for me! I'll take these points into
account next time.
Many other libc implementations have memrchr, but honestly, this third
commit was just something I noticed when reading the code, and not
directly related to the bug I was trying to fix.

Your se_implicit test case got left out of your commit. I guess this
was unintentional.
Presumably, the test case was based on [SV 54161]. The bug can now be closed.

Regards,
- Jouke

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Paul Smith-20
On Wed, 2019-12-18 at 16:02 +0100, Jouke Witteveen wrote:
> Your se_implicit test case got left out of your commit. I guess this
> was unintentional.

I'm not sure what you mean?  It's there...?

> Presumably, the test case was based on [SV 54161]. The bug can now be
> closed.

Yes, will do that later.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 2/3] [SV 54161] Fix second expansion of $*

Jouke Witteveen
On Wed, Dec 18, 2019 at 4:19 PM Paul Smith <[hidden email]> wrote:
>
> On Wed, 2019-12-18 at 16:02 +0100, Jouke Witteveen wrote:
> > Your se_implicit test case got left out of your commit. I guess this
> > was unintentional.
>
> I'm not sure what you mean?  It's there...?

My mistake. I must have done something wrong, because now I see it.

> > Presumably, the test case was based on [SV 54161]. The bug can now be
> > closed.
>
> Yes, will do that later.
>