bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[David Fussner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

The etags program reliably crashes for me when running the test suite in
test/manual/etags, but you need to have a long-enough TEXTAGS
environment variable to make it happen.  On my 32-bit GNU/Linux machine,
going to that directory and running:

TEXTAGS="A:B:C:D:E:F:G:H:I:J:K:L:M:N:O:P:Q:R:S:T:U:V:W:X" make check

crashes the program before it finishes the first run, that is, before it
stops because the test suite itself fails.  Removing one member of
TEXTAGS allows it to complete, whereupon the test suite (correctly)
fails in the normal way.  Adding members gives me different crash
messages.  (The crash may well be machine and compiler dependent, so if
it fails to crash then adding things to TEXTAGS may work.)

Whether the program completes or no, and so long as you have something
in TEXTAGS, running etags under valgrind always gives the following
messages:

==14977== Invalid write of size 4
==14977==    at 0x804EF32: TEX_decode_env (etags.c:5808)

==14977== Invalid write of size 4
==14977==    at 0x804EF39: TEX_decode_env (etags.c:5809)

Lines 5808-9 add NULL to end TEX_toktab, and the latter therefore needs
to have one more slot allotted than there are actual members of the
string made from the default defenv (defined in etags.c) and the TEXTAGS
environment variable.  When allocating storage, the code counts colons,
and since the default defenv begins with a colon, it always gets the
extra slot by default when there is no TEXTAGS environment variable (and
those messages no longer appear under valgrind).  Define a TEXTAGS
variable, which according to the documentation doesn't start with a
colon, and there will be one slot too few.  The patch tests for whether
the final concatenated string starts with a colon or not, and increments
the starting value of len in TEX_toktab if it doesn't.

(Apologies for the long report -- I was sort of assuming that this might
be difficult to reproduce.)

Thanks,

David.

In GNU Emacs 29.0.50 (build 3, i686-pc-linux-gnu, GTK+ Version 3.18.9,
cairo version 1.14.6)
 of 2021-12-11 built on newfont
Repository revision: 8c50016b100ec2c548ec90131e0f5fb5f4ebb5c1
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description: Slackware 14.2

Configured using:
 'configure
 PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/usr/local/share/pkgconfig:/usr/lib/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND
SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LC_COLLATE: C
  value of $LANG: en_US.ISO8859-1
  locale-coding-system: iso-latin-1-unix

Major mode: C/*l

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  shell-dirtrack-mode: t
  bug-reference-prog-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  abbrev-mode: t

Load-path shadows:
/home/dfussner/.emacs.d/elpa/transient-20210426.2141/transient hides
/home/dfussner/src/emacs/emacs/lisp/transient

Features:
(shadow sort mail-extr emacsbug sendmail magit-extras face-remap
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func imenu
magit-diff smerge-mode diff git-commit rx log-edit message yank-media
rmc puny dired dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr
mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log magit-core
magit-autorevert autorevert filenotify magit-margin magit-transient
magit-process with-editor magit-mode transient cl-extra help-mode
format-spec magit-git magit-section magit-utils crm dash misearch
multi-isearch shell pcomplete compile text-property-search comint
ansi-color ring vc-git diff-mode easy-mmode vc vc-dispatcher
bug-reference cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles
cc-align cc-engine cc-vars cc-defs doom-opera-theme doom-themes
doom-themes-base time-date server preview-latex auto-loads tex-site
finder-inf info package browse-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util mailcap
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map url-vars seq gv subr-x byte-opt
bytecomp byte-compile cconv cl-loaddefs cl-lib iso-transl tooltip eldoc
paren electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 8 373679 54043)
 (symbols 24 21995 1)
 (strings 16 103078 4463)
 (string-bytes 1 2950986)
 (vectors 8 45064)
 (vector-slots 4 1306902 95228)
 (floats 8 245 822)
 (intervals 28 7124 442)
 (buffers 564 20)
 (heap 1024 33446 1163))

[-- Attachment #2: 0001-Fix-off-by-one-error-in-etags.c-TeX-support.patch --]
[-- Type: text/x-patch, Size: 1115 bytes --]

From fc3a207074666650fee752359ba87c96a17f96b9 Mon Sep 17 00:00:00 2001
From: David Fussner <dfussner@googlemail.com>
Date: Sat, 11 Dec 2021 17:47:38 +0000
Subject: [PATCH] Fix off-by-one error in etags.c TeX support

* lib-src/etags.c (TEX_decode_env): Fix off-by-one error.
---
 lib-src/etags.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index bd4d4fcf53..37b4cda801 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -5773,6 +5773,7 @@ TeX_commands (FILE *inf)
 TEX_decode_env (const char *evarname, const char *defenv)
 {
   const char *env, *p;
+  char q = 1;
   ptrdiff_t len;
 
   /* Append default string to environment. */
@@ -5782,8 +5783,12 @@ TEX_decode_env (const char *evarname, const char *defenv)
   else
     env = concat (env, defenv, "");
 
+  /* Fix off-by-one error. */
+  if (!strneq (env, ":", 1))
+    q++;
+
   /* Allocate a token table */
-  for (len = 1, p = env; (p = strchr (p, ':')); )
+  for (len = q, p = env; (p = strchr (p, ':')); )
     if (*++p)
       len++;
   TEX_toktab = xnew (len, linebuffer);
-- 
2.17.6

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[Lars Ingebrigtsen]

David Fussner <dfussner@googlemail.com> writes:

> The etags program reliably crashes for me when running the test suite in
> test/manual/etags, but you need to have a long-enough TEXTAGS
> environment variable to make it happen.  On my 32-bit GNU/Linux machine,
> going to that directory and running:
>
> TEXTAGS="A:B:C:D:E:F:G:H:I:J:K:L:M:N:O:P:Q:R:S:T:U:V:W:X" make check
>
> crashes the program before it finishes the first run, that is, before it
> stops because the test suite itself fails. 

I'm not able to reproduce the issue on my 64-bit Debian system, but
reading the code, I think your analysis of the problem sounds correct.
The patch seemed slightly confusing, though, so I rewrote it a bit.
Could you check whether this version also fixes the problem?


diff --git a/lib-src/etags.c b/lib-src/etags.c
index bd4d4fcf53..ba66eeede4 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -5773,7 +5773,7 @@ TeX_commands (FILE *inf)
 TEX_decode_env (const char *evarname, const char *defenv)
 {
   const char *env, *p;
-  ptrdiff_t len;
+  ptrdiff_t len = 1;
 
   /* Append default string to environment. */
   env = getenv (evarname);
@@ -5782,8 +5782,13 @@ TEX_decode_env (const char *evarname, const char *defenv)
   else
     env = concat (env, defenv, "");
 
+  /* If the environment variable starts with a colon, increase the
+     length of the token table.  */
+  if (!strneq (env, ":", 1))
+    len++;
+
   /* Allocate a token table */
-  for (len = 1, p = env; (p = strchr (p, ':')); )
+  for (p = env; (p = strchr (p, ':')); )
     if (*++p)
       len++;
   TEX_toktab = xnew (len, linebuffer);


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[Andreas Schwab]

On Dez 12 2021, Lars Ingebrigtsen wrote:

> +  /* If the environment variable starts with a colon, increase the
> +     length of the token table.  */
> +  if (!strneq (env, ":", 1))

That's a pretty roundabout way to write *env == ':'.

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

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[David Fussner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Lars,

> I'm not able to reproduce the issue on my 64-bit Debian system, but
> reading the code, I think your analysis of the problem sounds correct.
> The patch seemed slightly confusing, though, so I rewrote it a bit.
> Could you check whether this version also fixes the problem?

If my code was merely confusing and inelegant rather than flat-out
incorrect then that's a triumph :-)

> diff --git a/lib-src/etags.c b/lib-src/etags.c
> index bd4d4fcf53..ba66eeede4 100644
> --- a/lib-src/etags.c
> +++ b/lib-src/etags.c
> @@ -5773,7 +5773,7 @@ TeX_commands (FILE *inf)
>  TEX_decode_env (const char *evarname, const char *defenv)
>  {
>    const char *env, *p;
> -  ptrdiff_t len;
> +  ptrdiff_t len = 1;
>
>    /* Append default string to environment. */
>    env = getenv (evarname);
> @@ -5782,8 +5782,13 @@ TEX_decode_env (const char *evarname, const char *defenv)
>    else
>      env = concat (env, defenv, "");
>
> +  /* If the environment variable starts with a colon, increase the
> +     length of the token table.  */
> +  if (!strneq (env, ":", 1))
> +    len++;
> +
>    /* Allocate a token table */
> -  for (len = 1, p = env; (p = strchr (p, ':')); )
> +  for (p = env; (p = strchr (p, ':')); )
>      if (*++p)
>        len++;
>    TEX_toktab = xnew (len, linebuffer);
>

That fixes the issue here, thank you very much! ( I guess Andreas'
comment on my strneq test is well-founded, though?)

Please close this bug if/when you apply your fix.

David.

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[Lars Ingebrigtsen]

David Fussner <dfussner@googlemail.com> writes:

> That fixes the issue here, thank you very much! ( I guess Andreas'
> comment on my strneq test is well-founded, though?)
>
> Please close this bug if/when you apply your fix.

Thanks for testing; I've now pushed that version of the patch (with
Andreas' change) to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[David Fussner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks again, Lars.

>
> Thanks for testing; I've now pushed that version of the patch (with
> Andreas' change) to Emacs 29.
>

David.

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[David Fussner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Sorry Lars -- I just looked at the diff and I think it should be:

if (*env != ':')

instead of:

if (*env == ':') ?

Or am I confused as usual?

David.

On Sun, 12 Dec 2021 at 10:29, David Fussner <dfussner@googlemail.com> wrote:
>
> Thanks again, Lars.
>
> >
> > Thanks for testing; I've now pushed that version of the patch (with
> > Andreas' change) to Emacs 29.
> >
>
> David.

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[David Fussner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

And again --

/* If the environment variable doesn't start with a colon, increase the
   length of the token table.  */

That was the logic of my patch, in any case, and of your previous one.

David.

bug#52438: 29.0.50; [PATCH] Fix off-by-one error in etags.c TeX support

[Lars Ingebrigtsen]

David Fussner <dfussner@googlemail.com> writes:

> Sorry Lars -- I just looked at the diff and I think it should be:
>
> if (*env != ':')
>
> instead of:
>
> if (*env == ':') ?

Yes, indeed.  :-/  Now re-fixed.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no