unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Fixing some warnings in emacs-26
@ 2019-06-08  5:11 Juanma Barranquero
  2019-06-08  6:41 ` Eli Zaretskii
  2019-06-08  8:22 ` Andreas Schwab
  0 siblings, 2 replies; 9+ messages in thread
From: Juanma Barranquero @ 2019-06-08  5:11 UTC (permalink / raw)
  To: Emacs developers

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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  5:11 Fixing some warnings in emacs-26 Juanma Barranquero
@ 2019-06-08  6:41 ` Eli Zaretskii
  2019-06-08  7:07   ` Juanma Barranquero
  2019-06-08  8:22 ` Andreas Schwab
  1 sibling, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-08  6:41 UTC (permalink / raw)
  To: Juanma Barranquero; +Cc: emacs-devel

> 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).



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  6:41 ` Eli Zaretskii
@ 2019-06-08  7:07   ` Juanma Barranquero
  2019-06-08  8:43     ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Juanma Barranquero @ 2019-06-08  7:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Emacs developers

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  5:11 Fixing some warnings in emacs-26 Juanma Barranquero
  2019-06-08  6:41 ` Eli Zaretskii
@ 2019-06-08  8:22 ` Andreas Schwab
  2019-06-08  8:51   ` Eli Zaretskii
  1 sibling, 1 reply; 9+ messages in thread
From: Andreas Schwab @ 2019-06-08  8:22 UTC (permalink / raw)
  To: Juanma Barranquero; +Cc: Emacs developers

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."



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  7:07   ` Juanma Barranquero
@ 2019-06-08  8:43     ` Eli Zaretskii
  2019-06-08 22:55       ` Juanma Barranquero
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-08  8:43 UTC (permalink / raw)
  To: Juanma Barranquero; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  8:22 ` Andreas Schwab
@ 2019-06-08  8:51   ` Eli Zaretskii
  2019-06-09 11:02     ` Juanma Barranquero
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-08  8:51 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: lekktu, emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  8:43     ` Eli Zaretskii
@ 2019-06-08 22:55       ` Juanma Barranquero
  2019-06-09  6:12         ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Juanma Barranquero @ 2019-06-08 22:55 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Emacs developers

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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08 22:55       ` Juanma Barranquero
@ 2019-06-09  6:12         ` Eli Zaretskii
  0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2019-06-09  6:12 UTC (permalink / raw)
  To: Juanma Barranquero; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fixing some warnings in emacs-26
  2019-06-08  8:51   ` Eli Zaretskii
@ 2019-06-09 11:02     ` Juanma Barranquero
  0 siblings, 0 replies; 9+ messages in thread
From: Juanma Barranquero @ 2019-06-09 11:02 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Andreas Schwab, Emacs developers

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);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-09 11:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-08  5:11 Fixing some warnings in emacs-26 Juanma Barranquero
2019-06-08  6:41 ` Eli Zaretskii
2019-06-08  7:07   ` Juanma Barranquero
2019-06-08  8:43     ` Eli Zaretskii
2019-06-08 22:55       ` Juanma Barranquero
2019-06-09  6:12         ` Eli Zaretskii
2019-06-08  8:22 ` Andreas Schwab
2019-06-08  8:51   ` Eli Zaretskii
2019-06-09 11:02     ` Juanma Barranquero

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).