bug#50946: Emacs-28: Inadequate coding in hack-elisp-shorthands

[Alan Mackenzie <acm@muc.de>]

Hello, João.

On Sat, Oct 02, 2021 at 01:48:31 +0100, João Távora wrote:
> Eli Zaretskii <eliz@gnu.org> writes:

> >> Date: Fri, 1 Oct 2021 17:10:57 +0000
> >> From: Alan Mackenzie <acm@muc.de>

> >> In emacs -Q in the emacs-28 branch, create the following two line file,
> >> foobar.el, and try to load it:

> >> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> >> (defvar foo-baz "foobar-baz")
> >> FOOBARELISP-SHORTHANDS: (("foo" . "foobar")))
> >> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

> >> This will throw an error, but that isn't important.

> >> What is important is that the symbol foobar-baz is created by the
> >> elisp-shorthands facility.

> >> This shouldn't happen since:
> >> 1/- There is no Local Variables section.
> >> 2/- There is no variable elisp-shorthands in that non-existent section.

> >> The following errors are evident in hack-elisp-shorthands:
> >> 1/- The code doesn't check for a correctly formatted Local Variables
> >>   section.
> >> 2/- The code, even if it did check, would only check the last 3000 bytes
> >>   in the file.  The section can occur anywhere in the last 3000
> >>   CHARACTERS.
> >> 3/- The code doesn't do a case-sensitive search for "elisp-shorthands".
> >> 4/- The code doesn't check for "elisp-shorthands" being a complete
> >>   symbol.
> >> 5/- The code doesn't even check that "elisp-shorthands" is in a comment.

> > Thanks.

> > João, could you please look into this?

> Done.

No you're not.  The bug is only partially fixed.  Why did you not check
your patch with me before closing the bug?  I've reopened it.

> In the Emacs 28 branch.  All tests pass (except a strange 'seccomp' one
> that never did).  Let me know if some more bugs lurk.

Not more bugs, just the original bug badly "fixed".

> Addressed all the points except the last one which doesn't make much
> sense, ....

You did address it, since hack-local-variables--find-variables checks for
the needed comment openers.

> .... since normal `hack-local-variables` also doesn't do any such
> check.  In fact what I'm doing is re-using
> hack-local-variables--find-variables from files.el, as I had wanted to
> anyway.

Why aren't you just using that function on the buffer anyway, instead of all this
clumsy messing around with temporary buffers, file-attributes, and
successive 100 bytes, and so on?

All right.  What is not fixed?  Say you have a file 3150 bytes long,
which is less than 3000 characters in Emacs.  Your function will load
only 3100 bytes, less than 3000 characters, into the temporary buffer.
It thus may fail to find a Local Variables section, even if this scenario
is highly unusual.

Have you checked that things work if the first byte in your temporary
buffer isn't at the start of a character?

Also, it is unclear whether the elisp-shorthands symbol on the first line
of a file is valid.  I think the intention is that it not be valid.
However, the current version of the code will read it from the first line
of a sufficiently small file.  You should look at this.  Possibly this
needs to be clarified in the documentation for shorthands.

I suggest you narrow the first line of the temporary buffer out in such
circumstances.  Better yet, dispense with all this clever stuff about
3000 bytes and 100 byte chunks, and just use
hack-local-variables--find-variables on the file's buffer (which you'll
have visited anyway, or shortly will visit)?

> João

-- 
Alan Mackenzie (Nuremberg, Germany).