GNU bug report logs - #28083
Two patches for grep on Windows, fixing directory recursion and leaking color

Previous Next

Package: grep;

Reported by: Stephan Lavavej <stl <at> nuwen.net>

Date: Sun, 13 Aug 2017 22:55:02 UTC

Severity: normal

Tags: patch

Done: Paul Eggert <eggert <at> cs.ucla.edu>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28083 in the body.
You can then email your comments to 28083 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Sun, 13 Aug 2017 22:55:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Stephan Lavavej <stl <at> nuwen.net>:
New bug report received and forwarded. Copy sent to bug-grep <at> gnu.org. (Sun, 13 Aug 2017 22:55:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stephan Lavavej <stl <at> nuwen.net>
To: bug-grep <at> gnu.org
Subject: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Sun, 13 Aug 2017 15:53:57 -0700
Hi,

I have two patches that improve grep's support for Windows. The first
patch is a one-liner that fixes directory recursion, while the second
patch fixes "leaking color" when grep is printing colorized output and
is terminated prematurely by Ctrl+C.

First patch: gnulib recently gained a module, windows-stat-inodes,
that fixes directory recursion on Windows. No changes to grep's C
sources are required - grep simply needs to request the module during
configuration. Here's how:

###

Add windows-stat-inodes to bootstrap.conf, fixing directory recursion
on Windows.

diff --git a/bootstrap.conf b/bootstrap.conf
index 1c50974..73f1573 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -93,6 +93,7 @@ wchar
 wcrtomb
 wctob
 wctype-h
+windows-stat-inodes
 xalloc
 xbinary-io
 xstrtoimax

###

Here's the gnulib commit that added the windows-stat-inodes module:
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=8123b614e616eaa951d842f10730ba7b914f75b3

When grep requests this module, its configure script will gain the
behavior that was implemented in windows-stat-inodes.m4. This detects
mingw and sets WINDOWS_STAT_INODES=1. All other platforms are
unaffected, setting WINDOWS_STAT_INODES=0 (which is what's happening
in the absence of this patch).

(Credit: Thanks to Pär Björklund who diagnosed the problem as
involving inodes, which led me to discover the gnulib module, and
thanks to Václav Haisman who provided the bootstrap.conf patch in
https://github.com/StephanTLavavej/mingw-distro/issues/41 .)


Second patch: grep implements colorized output on Windows with
SetConsoleTextAttribute(). However, if grep is terminated prematurely
by Ctrl+C when it's in the middle of printing colorized output, the
Windows console won't reset the color. This results in "leaking
color", where the prompt is displayed in whatever color that grep was
using for highlighting. The console's color can be restored by running
"color 07" (or whatever the normal color is), but the problem can be
avoided completely.

Fixing this problem is fairly simple. The WinAPI provides
SetConsoleCtrlHandler(), allowing a program to intercept Ctrl+C (and
Ctrl+Break, which is identically affected). On Windows, Ctrl+C works
by injecting an additional thread into the process. Note that my patch
isn't introducing this thread - it's always injected by Ctrl+C, which
uses a default handler to terminate the process. My patch simply adds
an additional handler to be called. So to avoid leaking color, we need
to:

1. Initialize a critical section, which cannot fail on Vista and
above. (If anyone cares about XP/2003, it can fail under conditions of
extremely low memory, which isn't a concern in practice.)

2. Whenever we're about to colorize the output, enter the critical
section and check a bool (initially true) saying whether we're allowed
to colorize. If so, call SetConsoleTextAttribute() while still inside
that critical section. If not, do nothing.

3. During initialization, use SetConsoleCtrlHandler() to add a small
function that will be called when Ctrl+C (or Ctrl+Break) is pressed.
This will enter the critical section, restore the console's text
color, and set the bool to false.

The critical section is necessary because when Windows injects the
thread for Ctrl+C, the main thread can be busy toggling the color on
and off. By resetting the text color and setting the bool to false,
the main thread is prevented from re-colorizing the console.

(The main thread also calls SetConsoleTextAttribute() at the end of
colorized regions. This doesn't need to be guarded by the critical
section, since repeatedly restoring the console's color is perfectly
fine.)

I've been using this patch for many years (going back to grep 2.10 in
March 2012 at least, when grep lacked support for color on Windows and
I added it locally). In this patch, I've tried to follow the
surrounding coding conventions to the best of my ability. As the
changes are limited to colorize-w32.c, no other platforms are
affected.

###

On Windows, restore the console's color when Ctrl+C is pressed.

diff --git a/lib/colorize-w32.c b/lib/colorize-w32.c
index 1073018..0ae014b 100644
--- a/lib/colorize-w32.c
+++ b/lib/colorize-w32.c
@@ -32,6 +32,27 @@

 static HANDLE hstdout = INVALID_HANDLE_VALUE;
 static SHORT norm_attr;
+static CRITICAL_SECTION color_lock;
+static BOOL color_bool = TRUE;
+
+/* After Ctrl+C or Ctrl+Break,
+   restore the normal text attribute used by the console.  */
+static BOOL WINAPI
+w32_color_handler (DWORD ctrl_type)
+{
+  if (ctrl_type == CTRL_C_EVENT
+      || ctrl_type == CTRL_BREAK_EVENT)
+    {
+      EnterCriticalSection (&color_lock);
+      if (hstdout != INVALID_HANDLE_VALUE)
+        {
+          SetConsoleTextAttribute (hstdout, norm_attr);
+        }
+      color_bool = FALSE;
+      LeaveCriticalSection (&color_lock);
+    }
+  return FALSE;
+}

 /* Initialize the normal text attribute used by the console.  */
 void
@@ -45,6 +66,9 @@ init_colorize (void)
      norm_attr = csbi.wAttributes;
   else
     hstdout = INVALID_HANDLE_VALUE;
+
+  InitializeCriticalSectionAndSpinCount (&color_lock, 4000);
+  SetConsoleCtrlHandler (&w32_color_handler, TRUE);
 }

 /* Return non-zero if we should highlight matches in output.  */
@@ -164,7 +188,12 @@ print_start_colorize (char const *sgr_start, char
const *sgr_seq)
   if (hstdout != INVALID_HANDLE_VALUE)
     {
       SHORT attr = w32_sgr2attr (sgr_seq);
-      SetConsoleTextAttribute (hstdout, attr);
+      EnterCriticalSection (&color_lock);
+      if (color_bool)
+        {
+          SetConsoleTextAttribute (hstdout, attr);
+        }
+      LeaveCriticalSection (&color_lock);
     }
   else
     printf (sgr_start, sgr_seq);

###

This patch is also available at:
https://github.com/StephanTLavavej/mingw-distro/blob/fbe3e6bcb952c8b48af0ebf4c0e13c08ffb8aea4/grep-lock.patch

Thank you very much for your continued maintenance of grep.

STL




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Sun, 26 Nov 2017 09:11:02 GMT) Full text and rfc822 format available.

Message #8 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Stephan Lavavej <stl <at> nuwen.net>
To: 28083 <at> debbugs.gnu.org
Subject: Re: bug#28083: Two patches for grep on Windows, fixing directory
 recursion and leaking color
Date: Sun, 26 Nov 2017 01:10:24 -0800
On Sun, Aug 13, 2017 at 3:53 PM, Stephan Lavavej <stl <at> nuwen.net> wrote:
> I have two patches that improve grep's support for Windows. The first
> patch is a one-liner that fixes directory recursion, while the second
> patch fixes "leaking color" when grep is printing colorized output and
> is terminated prematurely by Ctrl+C.

Is there anything else I can do to help these patches be accepted into master?

Thanks,
STL




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Mon, 27 Nov 2017 00:00:02 GMT) Full text and rfc822 format available.

Message #11 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Stephan Lavavej <stl <at> nuwen.net>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: bug#28083: Two patches for grep on Windows, fixing directory
 recursion and leaking color
Date: Sun, 26 Nov 2017 15:58:36 -0800
On Sun, Nov 26, 2017 at 1:10 AM, Stephan Lavavej <stl <at> nuwen.net> wrote:
> On Sun, Aug 13, 2017 at 3:53 PM, Stephan Lavavej <stl <at> nuwen.net> wrote:
>> I have two patches that improve grep's support for Windows. The first
>> patch is a one-liner that fixes directory recursion, while the second
>> patch fixes "leaking color" when grep is printing colorized output and
>> is terminated prematurely by Ctrl+C.
>
> Is there anything else I can do to help these patches be accepted into master?

[for reference, we're discussing these:
https://debbugs.gnu.org/db/28/28083.html]
Thank you for preparing those.

Would you please follow the contribution guidelines in grep's HACKING
file, https://git.savannah.gnu.org/cgit/grep.git/tree/HACKING to
create two git change sets? Since each fixes a bug, each should also
add a brief comment to the NEWS file.

Your first patch is tiny and looks fine, so I can apply it with no
paperwork required. However, the second is substantial enough that
before I can use it, you'll have to follow the instructions in the
"Copyright assignment" section of that same HACKING file.  Can you do
that?

Thanks,
Jim




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Mon, 27 Nov 2017 02:14:02 GMT) Full text and rfc822 format available.

Message #14 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Stephan Lavavej <stl <at> nuwen.net>
To: Jim Meyering <jim <at> meyering.net>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: bug#28083: Two patches for grep on Windows, fixing directory
 recursion and leaking color
Date: Sun, 26 Nov 2017 18:13:21 -0800
[Message part 1 (text/plain, inline)]
On Sun, Nov 26, 2017 at 3:58 PM, Jim Meyering <jim <at> meyering.net> wrote:
> [for reference, we're discussing these:
> https://debbugs.gnu.org/db/28/28083.html]
> Thank you for preparing those.

You're welcome!

> Would you please follow the contribution guidelines in grep's HACKING
> file, https://git.savannah.gnu.org/cgit/grep.git/tree/HACKING to
> create two git change sets? Since each fixes a bug, each should also
> add a brief comment to the NEWS file.
>
> Your first patch is tiny and looks fine, so I can apply it with no
> paperwork required.

Great, I've attached the output of git format-patch for that one.
Please let me know if you need it in a different form.

> However, the second is substantial enough that
> before I can use it, you'll have to follow the instructions in the
> "Copyright assignment" section of that same HACKING file.  Can you do
> that?

Sure, I'll start that process.

Thanks,
STL
[0001-grep-Fix-directory-recursion-on-MS-Windows.patch (application/octet-stream, attachment)]

Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Mon, 27 Nov 2017 02:59:01 GMT) Full text and rfc822 format available.

Message #17 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Stephan Lavavej <stl <at> nuwen.net>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: bug#28083: Two patches for grep on Windows, fixing directory
 recursion and leaking color
Date: Sun, 26 Nov 2017 18:58:19 -0800
On Sun, Nov 26, 2017 at 6:13 PM, Stephan Lavavej <stl <at> nuwen.net> wrote:
> On Sun, Nov 26, 2017 at 3:58 PM, Jim Meyering <jim <at> meyering.net> wrote:
>> [for reference, we're discussing these:
>> https://debbugs.gnu.org/db/28/28083.html]
>> Thank you for preparing those.
>
> You're welcome!
>
>> Would you please follow the contribution guidelines in grep's HACKING
>> file, https://git.savannah.gnu.org/cgit/grep.git/tree/HACKING to
>> create two git change sets? Since each fixes a bug, each should also
>> add a brief comment to the NEWS file.
>>
>> Your first patch is tiny and looks fine, so I can apply it with no
>> paperwork required.
>
> Great, I've attached the output of git format-patch for that one.
> Please let me know if you need it in a different form.

I made only these changes to the commit log before pushing that:
- removed capitalization and trailing period from the one-line summary
in the log message
- added ChangeLog-style "* FILENAME (function): Sentence/phrase
describing the change" line for each modified file.

Please follow suit for your next patch(es).
Thanks,
Jim




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Mon, 27 Nov 2017 04:20:02 GMT) Full text and rfc822 format available.

Message #20 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Stephan Lavavej <stl <at> nuwen.net>
To: Jim Meyering <jim <at> meyering.net>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: bug#28083: Two patches for grep on Windows, fixing directory
 recursion and leaking color
Date: Sun, 26 Nov 2017 20:19:21 -0800
On Sun, Nov 26, 2017 at 3:58 PM, Jim Meyering <jim <at> meyering.net> wrote:
> [for reference, we're discussing these:
> https://debbugs.gnu.org/db/28/28083.html]

On Sun, Nov 26, 2017 at 6:58 PM, Jim Meyering <jim <at> meyering.net> wrote:
> I made only these changes to the commit log before pushing that:
> - removed capitalization and trailing period from the one-line summary
> in the log message
> - added ChangeLog-style "* FILENAME (function): Sentence/phrase
> describing the change" line for each modified file.

Great, thank you so much!

> Please follow suit for your next patch(es).

Will do. Here's my current NEWS entry and commit message, if you're
interested. I've tried to follow the ChangeLog style. I'll send out a
git format-patch when the paperwork is complete.

NEWS entry:

  On MS-Windows, when grep is printing colorized output and is
  terminated prematurely by Ctrl+C, grep now restores the console's
  original color.
  [bug present since grep 2.11 added colorization for MS-Windows]

Commit message:

grep: reset MS-Windows console color after Ctrl+C

grep implements colorized output on MS-Windows with
SetConsoleTextAttribute(). However, if grep is terminated prematurely
by Ctrl+C when it's in the middle of printing colorized output, the
MS-Windows console won't reset the color. This results in "leaking
color", where the prompt is displayed in whatever color that grep was
using for highlighting. The console's color can be restored by running
"color 07" (or whatever the normal color is), but the problem can be
avoided completely.

Fixing this problem is fairly simple. The MS-Windows API provides
SetConsoleCtrlHandler(), allowing a program to intercept Ctrl+C (and
Ctrl+Break, which is identically affected). On MS-Windows, Ctrl+C works
by injecting an additional thread into the process. Note that this
patch isn't introducing this thread - it's always injected by Ctrl+C,
which uses a default handler to terminate the process. This patch
simply adds an additional handler to be called. So to avoid leaking
color, we need to:

1. Initialize a critical section, which cannot fail on MS-Windows Vista
and newer. (On older versions of MS-Windows, it can fail under
conditions of extremely low memory, which isn't a concern in practice.)

2. Whenever we're about to colorize the output, enter the critical
section and check a bool (initially true) saying whether we're allowed
to colorize. If so, call SetConsoleTextAttribute() while still inside
that critical section. If not, do nothing.

3. During initialization, use SetConsoleCtrlHandler() to add a small
function that will be called when Ctrl+C (or Ctrl+Break) is pressed.
This will enter the critical section, restore the console's text color,
and set the bool to false.

The critical section is necessary because when MS-Windows injects the
thread for Ctrl+C, the main thread can be busy toggling the color on
and off. By resetting the text color and setting the bool to false, the
main thread is prevented from re-colorizing the console.

(The main thread also calls SetConsoleTextAttribute() at the end of
colorized regions. This doesn't need to be guarded by the critical
section, since repeatedly restoring the console's color is perfectly
fine.)

As the changes are limited to colorize-w32.c, no other platforms are
affected.

* lib/colorize-w32.c (color_lock): Add static CRITICAL_SECTION.
(color_bool): Add static BOOL, initialized to TRUE.
(w32_color_handler): When handling a Ctrl+C or Ctrl+Break event, lock
color_lock, restore the console's color, and set color_bool to FALSE in
order to prevent re-colorization. Return FALSE, so the default handler
will be called next.
(init_colorize): After initializing hstdout and norm_attr, initialize
color_lock, then set w32_color_handler to handle signals for this
process.
(print_start_colorize): To prevent re-colorization, guard
SetConsoleTextAttribute by locking color_lock and testing color_bool.
* NEWS (Bug fixes): Mention this fix.

Thanks,
STL




Added tag(s) pending and patch. Request was from Paul Eggert <eggert <at> cs.ucla.edu> to control <at> debbugs.gnu.org. (Tue, 31 Dec 2019 19:42:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Tue, 31 Dec 2019 19:46:01 GMT) Full text and rfc822 format available.

Message #25 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stephan Lavavej <stl <at> nuwen.net>
Cc: Jim Meyering <jim <at> meyering.net>, 28083 <at> debbugs.gnu.org
Subject: Re: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Tue, 31 Dec 2019 11:45:50 -0800
Stephan, I'm following up on GNU grep Bug#28303 <https://debbugs.gnu.org/28083>
dated 2017-08-13, which says:

>> However, the second is substantial enough that
>> before I can use it, you'll have to follow the instructions in the
>> "Copyright assignment" section of that same HACKING file.  Can you do
>> that?
> 
> Sure, I'll start that process.

As far as I can tell that process never finished, which means your patch still
isn't in GNU grep. If you're still interested in getting it done please let us
know, and we can restart the process.




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Tue, 31 Dec 2019 20:45:02 GMT) Full text and rfc822 format available.

Message #28 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: "Stephan T. Lavavej" <stl <at> nuwen.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Jim Meyering <jim <at> meyering.net>, 28083 <at> debbugs.gnu.org
Subject: Re: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Tue, 31 Dec 2019 12:44:29 -0800
[Message part 1 (text/plain, inline)]
Hi Paul,

Thanks for the reminder; this got buried in my todo list and I didn't get
around to requesting the copyright assignment paperwork. I'd still like to
get this patch in, so let's restart. I kept my branch around and the patch
rebased cleanly (with a trivial merge to NEWS). I'll email the FSF this
week.

STL

On Tue, Dec 31, 2019 at 11:45 AM Paul Eggert <eggert <at> cs.ucla.edu> wrote:

> Stephan, I'm following up on GNU grep Bug#28303 <
> https://debbugs.gnu.org/28083>
> dated 2017-08-13, which says:
>
> >> However, the second is substantial enough that
> >> before I can use it, you'll have to follow the instructions in the
> >> "Copyright assignment" section of that same HACKING file.  Can you do
> >> that?
> >
> > Sure, I'll start that process.
>
> As far as I can tell that process never finished, which means your patch
> still
> isn't in GNU grep. If you're still interested in getting it done please
> let us
> know, and we can restart the process.
>
[Message part 2 (text/html, inline)]

Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Sun, 14 Nov 2021 21:35:02 GMT) Full text and rfc822 format available.

Message #31 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Stephan Lavavej <stl <at> nuwen.net>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Sun, 14 Nov 2021 13:34:52 -0800
Stephan, I'm following up on GNU grep Bug#28303 
<https://debbugs.gnu.org/28083> dated 2017-08-13, which ends with this 
email from you dated 2019-12-31:

> I'd still like to
> get this patch in, so let's restart. I kept my branch around and the patch
> rebased cleanly (with a trivial merge to NEWS). I'll email the FSF this
> week.

I don't see any record of your assigning copyright to the FSF since 
then, which means your patch still isn't in GNU grep. If you're still 
interested in getting it done please let us know, and we can restart the 
process again.




Information forwarded to bug-grep <at> gnu.org:
bug#28083; Package grep. (Wed, 17 Nov 2021 12:38:02 GMT) Full text and rfc822 format available.

Message #34 received at 28083 <at> debbugs.gnu.org (full text, mbox):

From: "Stephan T. Lavavej" <stl <at> nuwen.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28083 <at> debbugs.gnu.org
Subject: Re: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Wed, 17 Nov 2021 04:37:11 -0800
[Message part 1 (text/plain, inline)]
I found that the copyright assignment process was more burdensome than I
expected (in particular, the request for employer signoff), so I abandoned
the attempt. Please go ahead and close this bug, if nobody wants to attempt
to implement a fix from scratch.

Thanks (and apologies for the delayed reply),
STL

On Sun, Nov 14, 2021 at 1:34 PM Paul Eggert <eggert <at> cs.ucla.edu> wrote:

> Stephan, I'm following up on GNU grep Bug#28303
> <https://debbugs.gnu.org/28083> dated 2017-08-13, which ends with this
> email from you dated 2019-12-31:
>
> > I'd still like to
> > get this patch in, so let's restart. I kept my branch around and the
> patch
> > rebased cleanly (with a trivial merge to NEWS). I'll email the FSF this
> > week.
>
> I don't see any record of your assigning copyright to the FSF since
> then, which means your patch still isn't in GNU grep. If you're still
> interested in getting it done please let us know, and we can restart the
> process again.
>
[Message part 2 (text/html, inline)]

Reply sent to Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility. (Wed, 17 Nov 2021 20:35:02 GMT) Full text and rfc822 format available.

Notification sent to Stephan Lavavej <stl <at> nuwen.net>:
bug acknowledged by developer. (Wed, 17 Nov 2021 20:35:02 GMT) Full text and rfc822 format available.

Message #39 received at 28083-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: "Stephan T. Lavavej" <stl <at> nuwen.net>
Cc: 28083-done <at> debbugs.gnu.org
Subject: Re: Two patches for grep on Windows, fixing directory recursion and
 leaking color
Date: Wed, 17 Nov 2021 12:34:21 -0800
On 11/17/21 04:37, Stephan T. Lavavej wrote:
> I found that the copyright assignment process was more burdensome than I
> expected (in particular, the request for employer signoff), so I abandoned
> the attempt.

Oh well. Thanks for letting us know. Closing the bug report.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 16 Dec 2021 12:24:11 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 93 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.