Fixing some warnings in emacs-26

Fixing some warnings in emacs-26

[Juanma Barranquero]

I'd like to propose these two patches for the emacs-26 release branch.

The first one fixes a real (if perhaps unlikely) buffer overflow in addpm.c.

The second one just adds a couple of defvars and marks some unused
args with _underscore to silence a few lexical warnings.



* nt/addpm.c (main): Fix buffer overflow

---
 nt/addpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nt/addpm.c b/nt/addpm.c
index f71ce5f238..5ef8079101 100644
--- a/nt/addpm.c
+++ b/nt/addpm.c
@@ -219,7 +219,7 @@ main (int argc, char *argv[])
  {
    int result;

-   char msg[ MAX_PATH ];
+   char msg[ MAX_PATH + 20 ]; /* Needs extra space for fixed text.  */
    sprintf (msg, "Install Emacs at %s?\n", emacs_path);
    result = MessageBox (NULL, msg, "Install Emacs",
         MB_OKCANCEL | MB_ICONQUESTION);
-- 
2.21.0.windows.1

----------------------------------------------------

lisp/*.el: Fix some lexical warnings

* lisp/electric.el (electric-pair-text-pairs): Declare.
* lisp/url/url-auth.el (url-digest-auth-nonce-count):
Mark argument `nonce' as unused.
* lisp/ses.el (ses--edit-cell-completion-at-point-function)
(ses--read-printer-completion-at-point-function):
Mark argument `val' as unused.
* lisp/eshell/em-dirs.el (eshell-dirs-initialize, eshell/pwd):
Mark argument `args' as unused.
* lisp/term/ns-win.el (ns-version-string): Declare.
---
 lisp/electric.el       | 2 ++
 lisp/eshell/em-dirs.el | 4 ++--
 lisp/ses.el            | 4 ++--
 lisp/term/ns-win.el    | 2 ++
 lisp/url/url-auth.el   | 2 +-
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index a30090d1d8..6eb516a0b9 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -461,6 +461,8 @@ electric-quote-inhibit-functions
 after the inserted character.  The functions in this hook should
 not move point or change the current buffer.")

+(defvar electric-pair-text-pairs) ; from elec-pair.el
+
 (defun electric-quote-post-self-insert-function ()
   "Function that `electric-quote-mode' adds to `post-self-insert-hook'.
 This requotes when a quoting key is typed."
diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
index 4d8debb954..f22c49dad5 100644
--- a/lisp/eshell/em-dirs.el
+++ b/lisp/eshell/em-dirs.el
@@ -207,7 +207,7 @@ eshell-dirs-initialize
   (when eshell-cd-on-directory
     (make-local-variable 'eshell-interpreter-alist)
     (setq eshell-interpreter-alist
-   (cons (cons #'(lambda (file args)
+   (cons (cons #'(lambda (file _args)
                           (eshell-lone-directory-p file))
        'eshell-dirs-substitute-cd)
  eshell-interpreter-alist)))
@@ -300,7 +300,7 @@ eshell-complete-user-reference
      (file-name-as-directory (cdr user))))
  eshell-user-names)))))))

-(defun eshell/pwd (&rest args)
+(defun eshell/pwd (&rest _args)
   "Change output from `pwd' to be cleaner."
   (let* ((path default-directory)
  (len (length path)))
diff --git a/lisp/ses.el b/lisp/ses.el
index f3de00427b..095a20f829 100644
--- a/lisp/ses.el
+++ b/lisp/ses.el
@@ -2495,7 +2495,7 @@ ses--edit-cell-completion-at-point-function
          prefix-length)
     (when (and prefix (null (string= prefix "")))
       (setq prefix-length (length prefix))
-      (maphash (lambda (key val)
+      (maphash (lambda (key _val)
                  (let ((key-name (symbol-name key)))
                    (when (and (>= (length key-name) prefix-length)
                               (string= prefix (substring key-name 0
prefix-length)))
@@ -2648,7 +2648,7 @@ ses--read-printer-completion-at-point-function
          prefix-length)
     (when prefix
       (setq prefix-length (length prefix))
-      (maphash (lambda (key val)
+      (maphash (lambda (key _val)
                  (let ((key-name (symbol-name key)))
                    (when (and (>= (length key-name) prefix-length)
                               (string= prefix (substring key-name 0
prefix-length)))
diff --git a/lisp/term/ns-win.el b/lisp/term/ns-win.el
index 40397fcfed..7f7a341f4a 100644
--- a/lisp/term/ns-win.el
+++ b/lisp/term/ns-win.el
@@ -739,6 +739,8 @@ ns-paste-secondary
 ;;;; macOS-like defaults for trackpad and mouse wheel scrolling on
 ;;;; macOS 10.7+.

+(defvar ns-version-string)  ; nsfns.m
+
 ;; FIXME: This doesn't look right.  Is there a better way to do this
 ;; that keeps customize happy?
 (when (featurep 'cocoa)
diff --git a/lisp/url/url-auth.el b/lisp/url/url-auth.el
index c3714f2656..9f1ca49a59 100644
--- a/lisp/url/url-auth.el
+++ b/lisp/url/url-auth.el
@@ -194,7 +194,7 @@ url-digest-auth-make-cnonce
   (base64-encode-string
    (apply 'format "%016x%04x%04x%05x%05x" (random) (current-time)) t))

-(defun url-digest-auth-nonce-count (nonce)
+(defun url-digest-auth-nonce-count (_nonce)
   "The number requests sent to server with the given NONCE.
 This count includes the request we're preparing here.

-- 
2.21.0.windows.1

Re: Fixing some warnings in emacs-26

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 8 Jun 2019 07:11:01 +0200
> 
> I'd like to propose these two patches for the emacs-26 release branch.

Thanks.

> The first one fixes a real (if perhaps unlikely) buffer overflow in addpm.c.

Thanks for catching that, see a comment below.

> The second one just adds a couple of defvars and marks some unused
> args with _underscore to silence a few lexical warnings.

LGTM, thanks.  However, please install those on master, as I'd like to
minimize changes on the release branch: we will start the pretest of
Emacs 26.3 soon.

In general, cleanups that have no real bearing on the code should be
installed on master.

> --- a/nt/addpm.c
> +++ b/nt/addpm.c
> @@ -219,7 +219,7 @@ main (int argc, char *argv[])
>   {
>     int result;
> 
> -   char msg[ MAX_PATH ];
> +   char msg[ MAX_PATH + 20 ]; /* Needs extra space for fixed text.  */
>     sprintf (msg, "Install Emacs at %s?\n", emacs_path);

Instead of the arbitrary value of 20, I'd prefer using sizeof for the
actual text (which should probably be assigned to a variable, to avoid
mentioning the same text twice).

Re: Fixing some warnings in emacs-26

[Juanma Barranquero]

> LGTM, thanks.  However, please install those on master, as I'd like to
> minimize changes on the release branch: we will start the pretest of
> Emacs 26.3 soon.

OK

> Instead of the arbitrary value of 20, I'd prefer using sizeof for the
> actual text (which should probably be assigned to a variable, to avoid
> mentioning the same text twice).

Done.

Though I wanted to avoid adding complexity for a string message that's
unlikely to change. Also, addpm.c already contains a few instances of
hardcoded constants:

      /* Ensure there is enough room for "...\GNU Emacs\Emacs.lnk".  */
      if (strlen (start_folder) < (MAX_PATH - 20))

and

      char add_item[MAX_PATH*2 + 100];

etc.

Re: Fixing some warnings in emacs-26

[Andreas Schwab]

On Jun 08 2019, Juanma Barranquero <lekktu@gmail.com> wrote:

> diff --git a/nt/addpm.c b/nt/addpm.c
> index f71ce5f238..5ef8079101 100644
> --- a/nt/addpm.c
> +++ b/nt/addpm.c
> @@ -219,7 +219,7 @@ main (int argc, char *argv[])
>   {
>     int result;
>
> -   char msg[ MAX_PATH ];
> +   char msg[ MAX_PATH + 20 ]; /* Needs extra space for fixed text.  */
>     sprintf (msg, "Install Emacs at %s?\n", emacs_path);

emacs_path can be longer than MAX_PATH.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Fixing some warnings in emacs-26

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 8 Jun 2019 09:07:43 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > Instead of the arbitrary value of 20, I'd prefer using sizeof for the
> > actual text (which should probably be assigned to a variable, to avoid
> > mentioning the same text twice).
> 
> Done.
> 
> Though I wanted to avoid adding complexity for a string message that's
> unlikely to change. Also, addpm.c already contains a few instances of
> hardcoded constants:
> 
>       /* Ensure there is enough room for "...\GNU Emacs\Emacs.lnk".  */
>       if (strlen (start_folder) < (MAX_PATH - 20))
> 
> and
> 
>       char add_item[MAX_PATH*2 + 100];
> 
> etc.

Feel free to change them as well, although addpm is deprecated, so I
wonder whether we should bother cleaning up purely stylistic issues in
it.

Thanks.

Re: Fixing some warnings in emacs-26

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Sat, 08 Jun 2019 10:22:19 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > -   char msg[ MAX_PATH ];
> > +   char msg[ MAX_PATH + 20 ]; /* Needs extra space for fixed text.  */
> >     sprintf (msg, "Install Emacs at %s?\n", emacs_path);
> 
> emacs_path can be longer than MAX_PATH.

Not in the branch where the above code is used, I think.  In that
branch, emacs_path comes from the GetModuleFileName call, and we limit
the result from that call to MAX_PATH characters.

Re: Fixing some warnings in emacs-26

[Juanma Barranquero]

On Sat, Jun 8, 2019 at 10:43 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Feel free to change them as well

My point was that MAX_PATH + 20 wasn't particularly weird or ugly in
that context. But certainly changing addpm.c for stylistic reasons is
not worth the effort.

Re: Fixing some warnings in emacs-26

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sun, 9 Jun 2019 00:55:12 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> On Sat, Jun 8, 2019 at 10:43 AM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Feel free to change them as well
> 
> My point was that MAX_PATH + 20 wasn't particularly weird or ugly in
> that context.

No, but whenever we change something, I'd prefer to do it right.

> But certainly changing addpm.c for stylistic reasons is not worth
> the effort.

Agreed.

Re: Fixing some warnings in emacs-26

[Juanma Barranquero]

On Sat, Jun 8, 2019 at 10:51 AM Eli Zaretskii <eliz@gnu.org> wrote:

> In that
> branch, emacs_path comes from the GetModuleFileName call, and we limit
> the result from that call to MAX_PATH characters.

Correct, and that's why the compiler warning was so explicit:

addpm.c:223:36:
    sprintf (msg, "Install Emacs at %s?\n", emacs_path);
                                    ^~
addpm.c:223:4: note: 'sprintf' output between 20 and 279 bytes into a
destination of size 260
    sprintf (msg, "Install Emacs at %s?\n", emacs_path);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~