bug#28083: Two patches for grep on Windows, fixing directory recursion and leaking color

[Stephan Lavavej]

On Sun, Nov 26, 2017 at 3:58 PM, Jim Meyering <address@hidden> 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 <address@hidden> 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