bug#32189: 27.0.50; GCC 7 warning due to -Wformat-truncation=2

[Paul Eggert <eggert@cs.ucla.edu>]

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

Eli Zaretskii wrote:
> Thanks, this LGTM, but I'd like to hear Paul's opinion about this
> warning before we push (since this should probably go to the emacs-26
> branch?).

Let's continue using -Wformat-truncation=2 since I recall it finding real bugs 
in Emacs in the past, and there's a better fix for this particular problem.

Some background. Generally I don't worry too much about warnings from older 
compilers, since one can just compile with --disable-gcc-warnings if the 
compiler is too old. It's better to use the latest GCC to find bugs, and not 
worry about pacifying older GCCs that are somewhat flaky in this area (as 
pacifying them would be a neverending project with little benefit).

Two comments for this particular case. First, Emacs generally prefers sprintf to 
snprintf, because truncation (which is what the latter does) is often just as 
serious a bug as buffer overflow (which is what the former does), and the GNU 
coding style is to avoid both bugs in which case sprintf is generally simpler 
and easier to use. I realize this goes against the common wisdom that snprintf 
is "safer" than sprintf, but the common wisdom is typically wrong for 
high-quality code. In the few places where Emacs does use snprintf, it either 
checks for truncated output and reports an error if so (which is lame, but at 
least there's an error check), or truncation is expected and is OK.

Second, the code in question uses snprintf followed by build_string, and can be 
simplified by using vformat_string instead. That would avoid the warnings and 
should make the code more reliable. Something like the attached patch, say. I 
haven't tested it since I don't use MS-Windows. Let's use something like this 
rather than shutting off the warnings.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Simplify-w32cygwinx.c-and-pacify-GCC-Bug-32189.patch --]
[-- Type: text/x-patch; name="0001-Simplify-w32cygwinx.c-and-pacify-GCC-Bug-32189.patch", Size: 2322 bytes --]

From 95ed210f17a7970764fb670e8b5d41b3d2ff3084 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 19 Jul 2018 16:12:17 -0700
Subject: [PATCH] Simplify w32cygwinx.c and pacify GCC (Bug#32189)

* src/w32cygwinx.c (format_string): New function.
(Fw32_battery_status): Use it.
---
 src/w32cygwinx.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/src/w32cygwinx.c b/src/w32cygwinx.c
index 8d3ae16..044e1fc 100644
--- a/src/w32cygwinx.c
+++ b/src/w32cygwinx.c
@@ -24,6 +24,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "lisp.h"
 #include "w32common.h"
 
+static Lisp_Object
+format_string (char const *format, ...)
+{
+  va_list args;
+  va_start (args, format);
+  Lisp_Object str = vformat_string (format, args);
+  va_end (args);
+  return str;
+}
+
 DEFUN ("w32-battery-status", Fw32_battery_status, Sw32_battery_status, 0, 0, 0,
        doc: /* Get power status information from Windows system.
 
@@ -92,32 +102,17 @@ The following %-sequences are provided:
       if (system_status.BatteryLifePercent > 100)
 	load_percentage = build_string ("N/A");
       else
-	{
-	  char buffer[16];
-	  snprintf (buffer, 16, "%d", system_status.BatteryLifePercent);
-	  load_percentage = build_string (buffer);
-	}
+	load_percentage = format_string ("%d", system_status.BatteryLifePercent);
 
       if (seconds_left < 0)
 	seconds = minutes = hours = remain = build_string ("N/A");
       else
 	{
-	  long m;
-	  double h;
-	  char buffer[16];
-	  snprintf (buffer, 16, "%ld", seconds_left);
-	  seconds = build_string (buffer);
-
-	  m = seconds_left / 60;
-	  snprintf (buffer, 16, "%ld", m);
-	  minutes = build_string (buffer);
-
-	  h = seconds_left / 3600.0;
-	  snprintf (buffer, 16, "%3.1f", h);
-	  hours = build_string (buffer);
-
-	  snprintf (buffer, 16, "%ld:%02ld", m / 60, m % 60);
-	  remain = build_string (buffer);
+	  long m = seconds_left / 60;
+	  seconds = format_string ("%ld", seconds_left);
+	  minutes = format_string ("%ld", m);
+	  hours = format_string ("%3.1f", seconds_left / 3600.0);
+	  remain = format_string ("%ld:%02ld", m / 60, m % 60);
 	}
 
       status = listn (CONSTYPE_HEAP, 8,
-- 
2.7.4