bytecomp.el

31/10/2017

It’s halloween, so here’s a real treat for you, the commentary in bytecomp.el! The author of that piece of code is Jamie Zawinski who did invaluable work for both GNU Emacs and XEmacs, these days he runs a night club and blogs. Here are my favorite parts of the file:

  • ";; We successfully didn't compile this file."
    
  • (insert "\n") ; aaah, unix.
    
  •             (when old-style-backquotes
                  (byte-compile-warn "!! The file uses old-style backquotes !!
    This functionality has been obsolete for more than 10 years already
    and will be removed soon.  See (elisp)Backquote in the manual."))
    
  • ;; Insert semicolons as ballast, so that byte-compile-fix-header
    ;; can delete them so as to keep the buffer positions
    ;; constant for the actual compiled code.
    ";;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;\n"
    ";;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;\n\n"
    
  • ;; To avoid consing up monstrously large forms at load time, we split
    ;; the output regularly.
    
  • ;; If things not being bound at all is ok, so must them being
    ;; obsolete.  Note that we add to the existing lists since Tramp
    ;; (ab)uses this feature.
    
  • ;; If foo.el declares `toto' as obsolete, it is likely that foo.el will
    ;; actually use `toto' in order for this obsolete variable to still work
    ;; correctly, so paradoxically, while byte-compiling foo.el, the presence
    ;; of a make-obsolete-variable call for `toto' is an indication that `toto'
    ;; should not trigger obsolete-warnings in foo.el.
    
  • ;; FIXME: we also use this hunk-handler to implement the function's dynamic
    ;; docstring feature.  We could actually implement it more elegantly in
    ;; byte-compile-lambda so it applies to all lambdas, but the problem is that
    ;; the resulting .elc format will not be recognized by make-docfile, so
    ;; either we stop using DOC for the docstrings of preloaded elc files (at the
    ;; cost of around 24KB on 32bit hosts, double on 64bit hosts) or we need to
    ;; build DOC in a more clever way (e.g. handle anonymous elements).
    
  • ;; Don't reload the source version of the files below
    ;; because that causes subsequent byte-compilation to
    ;; be a lot slower and need a higher max-lisp-eval-depth,
    ;; so it can cause recompilation to fail.
    
  • ;; To avoid "lisp nesting exceeds max-lisp-eval-depth" when bytecomp compiles
    ;; itself, compile some of its most used recursive functions (at load time).
    

Don’t get me wrong, I’m aware that these are all necessary and don’t indicate deeper faults in the source code. I merely find it interesting what hacks one has to come up with for byte-code compilation and found studying the file enlightening to say the least.


Unjustified Indirection

19/10/2017

I finally made that EPUB mode. This adventure mostly taught me that eww, or rather, shr.el isn’t quite reusable. That itself is not really a problem, but I handed in a patch to improve the situation. An old saying among programmers is that every problem can be solved by applying an extra level of indirection, so that’s what I did after discussing it out on the bug tracker, however after my patch got merged it was deemed too much:

;; We don't use shr-indirect-call here, since shr-descend is
;; the central bit of shr.el, and should be as fast as
;; possible.  Having one more level of indirection with its
;; negative effect on performance is deemed unjustified in
;; this case.

Hadn’t I spoken up about inclusion of this comment, an unsuspecting future hacker wouldn’t even know why there’s duplicated code not using the helper. I can only wonder how production-ready browser engines solve this kind of problem…


make-temp-name

18/07/2017

Update: Reddit points out that this has been fixed on master by replacing most of the code with a call to gnulib’s gen_tempname.

For someone not terribly experienced in writing safe programs, one can only hope that building blocks like make-temp-file are doing the right thing and cannot be subverted by a malicious third party. The general advice here is that it’s preferable to use the primitive for creating the temporary file instead of the primitive to generate its name. Now, does Emacs reuse mkstemp(3) for this? Or at least tmpnam(3)? Of course not! Where we go, we can just invent our own source of randomness:

make-temp-file looks as follows:

static const char make_temp_name_tbl[64] =
{
  'A','B','C','D','E','F','G','H',
  'I','J','K','L','M','N','O','P',
  'Q','R','S','T','U','V','W','X',
  'Y','Z','a','b','c','d','e','f',
  'g','h','i','j','k','l','m','n',
  'o','p','q','r','s','t','u','v',
  'w','x','y','z','0','1','2','3',
  '4','5','6','7','8','9','-','_'
};

static unsigned make_temp_name_count, make_temp_name_count_initialized_p;

/* Value is a temporary file name starting with PREFIX, a string.

   The Emacs process number forms part of the result, so there is
   no danger of generating a name being used by another process.
   In addition, this function makes an attempt to choose a name
   which has no existing file.  To make this work, PREFIX should be
   an absolute file name.

   BASE64_P means add the pid as 3 characters in base64
   encoding.  In this case, 6 characters will be added to PREFIX to
   form the file name.  Otherwise, if Emacs is running on a system
   with long file names, add the pid as a decimal number.

   This function signals an error if no unique file name could be
   generated.  */

Lisp_Object
make_temp_name (Lisp_Object prefix, bool base64_p)
{
  Lisp_Object val, encoded_prefix;
  ptrdiff_t len;
  printmax_t pid;
  char *p, *data;
  char pidbuf[INT_BUFSIZE_BOUND (printmax_t)];
  int pidlen;

  CHECK_STRING (prefix);

  /* VAL is created by adding 6 characters to PREFIX.  The first
     three are the PID of this process, in base 64, and the second
     three are incremented if the file already exists.  This ensures
     262144 unique file names per PID per PREFIX.  */

  pid = getpid ();

  if (base64_p)
    {
      pidbuf[0] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidbuf[1] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidbuf[2] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidlen = 3;
    }
  else
    {
#ifdef HAVE_LONG_FILE_NAMES
      pidlen = sprintf (pidbuf, "%"pMd, pid);
#else
      pidbuf[0] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidbuf[1] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidbuf[2] = make_temp_name_tbl[pid & 63], pid >>= 6;
      pidlen = 3;
#endif
    }

  encoded_prefix = ENCODE_FILE (prefix);
  len = SBYTES (encoded_prefix);
  val = make_uninit_string (len + 3 + pidlen);
  data = SSDATA (val);
  memcpy (data, SSDATA (encoded_prefix), len);
  p = data + len;

  memcpy (p, pidbuf, pidlen);
  p += pidlen;

  /* Here we try to minimize useless stat'ing when this function is
     invoked many times successively with the same PREFIX.  We achieve
     this by initializing count to a random value, and incrementing it
     afterwards.

     We don't want make-temp-name to be called while dumping,
     because then make_temp_name_count_initialized_p would get set
     and then make_temp_name_count would not be set when Emacs starts.  */

  if (!make_temp_name_count_initialized_p)
    {
      make_temp_name_count = time (NULL);
      make_temp_name_count_initialized_p = 1;
    }

  while (1)
    {
      unsigned num = make_temp_name_count;

      p[0] = make_temp_name_tbl[num & 63], num >>= 6;
      p[1] = make_temp_name_tbl[num & 63], num >>= 6;
      p[2] = make_temp_name_tbl[num & 63], num >>= 6;

      /* Poor man's congruential RN generator.  Replace with
         ++make_temp_name_count for debugging.  */
      make_temp_name_count += 25229;
      make_temp_name_count %= 225307;

      if (!check_existing (data))
        {
          /* We want to return only if errno is ENOENT.  */
          if (errno == ENOENT)
            return DECODE_FILE (val);
          else
            /* The error here is dubious, but there is little else we
               can do.  The alternatives are to return nil, which is
               as bad as (and in many cases worse than) throwing the
               error, or to ignore the error, which will likely result
               in looping through 225307 stat's, which is not only
               dog-slow, but also useless since eventually nil would
               have to be returned anyway.  */
            report_file_error ("Cannot create temporary name for prefix",
                               prefix);
          /* not reached */
        }
    }
}

DEFUN ("make-temp-name", Fmake_temp_name, Smake_temp_name, 1, 1, 0,
       doc: /* Generate temporary file name (string) starting with PREFIX (a string).
The Emacs process number forms part of the result, so there is no
danger of generating a name being used by another Emacs process
\(so long as only a single host can access the containing directory...).

This function tries to choose a name that has no existing file.
For this to work, PREFIX should be an absolute file name.

There is a race condition between calling `make-temp-name' and creating the
file, which opens all kinds of security holes.  For that reason, you should
normally use `make-temp-file' instead.  */)
  (Lisp_Object prefix)
{
  return make_temp_name (prefix, 0);
}

The generated file name is therefore a combination of the prefix, the Emacs PID and three characters from the above table. This makes about 200.000 possible temporary files that can be generated with a given prefix in an Emacs session. This range can be traversed in a negligible amount of time to recreate the state of the RNG and accurately predict the next temporary file name.

(defun make-temp-file (prefix &optional dir-flag suffix)
  "Create a temporary file.
The returned file name (created by appending some random characters at the end
of PREFIX, and expanding against `temporary-file-directory' if necessary),
is guaranteed to point to a newly created empty file.
You can then use `write-region' to write new data into the file.

If DIR-FLAG is non-nil, create a new empty directory instead of a file.

If SUFFIX is non-nil, add that at the end of the file name."
  ;; Create temp files with strict access rights.  It's easy to
  ;; loosen them later, whereas it's impossible to close the
  ;; time-window of loose permissions otherwise.
  (with-file-modes ?\700
    (let (file)
      (while (condition-case ()
                 (progn
                   (setq file
                         (make-temp-name
                          (if (zerop (length prefix))
                              (file-name-as-directory
                               temporary-file-directory)
                            (expand-file-name prefix
                                              temporary-file-directory))))
                   (if suffix
                       (setq file (concat file suffix)))
                   (if dir-flag
                       (make-directory file)
                     (write-region "" nil file nil 'silent nil 'excl))
                   nil)
               (file-already-exists t))
        ;; the file was somehow created by someone else between
        ;; `make-temp-name' and `write-region', let's try again.
        nil)
      file)))

It’s interesting that the docstring of this function states that the return value “is guaranteed to point to a newly created empty file.”. If there were to exist a file for every possible combination for a prefix, this function would just fall into an infinite loop and block Emacs for no apparent reason. Both of these issues have been solved in a better way in glibc.

At least the impact of predicting the name is lessened if one uses make-temp-file instead of make-temp-name on its own. An attacker cannot create a symlink pointing to a rogue location with the predicted name as that would trigger a file-already-exists error and make the function use the next random name. All they could do is read out the file afterwards iff they have the same permission as the user Emacs runs with. A symlink attack can only be executed successfully with a careless make-temp-name user, thankfully I’ve not been able to find one worth subverting on GitHub yet.

Thanks to dale on #emacs for bringing this to my attention!


The Dude Abides

15/05/2017

While crafting another patch for cc-mode, I stumbled upon the following:

;; RMS says don't make these the default.
;; (April 2006): RMS has now approved these commands as defaults.
(unless (memq 'argumentative-bod-function c-emacs-features)
  (define-key c-mode-base-map "\e\C-a"    'c-beginning-of-defun)
  (define-key c-mode-base-map "\e\C-e"    'c-end-of-defun))

I’ve seen this kind of thing before, but it keeps puzzling me. Why would you:

  • Put a comment there about not making the following a default
  • Put a clarifying comment how these have been approved as defaults now
  • Keep both comments to trip up future readers

Any other project I’m involved in would immediately remove that commentary. Emacs is the only one where suggesting such drastic action would yield a lengthy bikeshedding discussion about the merits of pre-VCS traditions and how keeping them will toughen up whoever may join in the collaborative development effort. Yes, seriously.


Less Than Exceptional

26/02/2017

As of a few weeks ago, I’ve become part of Evil’s new maintainer team. Most of the bugs we get are not quite Evil’s fault, but rather that of other components it relies upon, such as Emacs. #785 is a perfect example of this observation. Take a look at the sources of the function responsible for the reported behavior and see whether you can find what’s wrong about it:

DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
       doc: ...)
  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
{
  ptrdiff_t idx;
  Lisp_Object cmd;
  Lisp_Object c;
  ptrdiff_t length;
  bool t_ok = !NILP (accept_default);

  keymap = get_keymap (keymap, 1, 1);

  length = CHECK_VECTOR_OR_STRING (key);
  if (length == 0)
    return keymap;

  idx = 0;
  while (1)
    {
      c = Faref (key, make_number (idx++));

      if (CONSP (c) && lucid_event_type_list_p (c))
        c = Fevent_convert_list (c);

      /* Turn the 8th bit of string chars into a meta modifier.  */
      if (STRINGP (key) && XINT (c) & 0x80 && !STRING_MULTIBYTE (key))
        XSETINT (c, (XINT (c) | meta_modifier) & ~0x80);

      /* Allow string since binding for `menu-bar-select-buffer'
         includes the buffer name in the key sequence.  */
      if (!INTEGERP (c) && !SYMBOLP (c) && !CONSP (c) && !STRINGP (c))
        message_with_string ("Key sequence contains invalid event %s", c, 1);

      cmd = access_keymap (keymap, c, t_ok, 0, 1);
      if (idx == length)
        return cmd;

      keymap = get_keymap (cmd, 0, 1);
      if (!CONSP (keymap))
        return make_number (idx);

      maybe_quit ();
    }
}

If you haven’t spotted it, the function never signals an exception for the exceptional case of an invalid key sequence. Instead, it returns one of the few types that aren’t allowed in a definition, namely an integer. It’s up to the caller to handle this case on their own, something the majority of Emacs packages rightfully neglect to do as even + informs you about invalid input. This design decision reeks of the many ways of signalling errors in C and worse, cannot be properly fixed as there’s code inside Emacs relying on the integer return type. There is even a lookup-key-ignore-too-long in Emacs core that looks a lot like what I wrote, but it’s part of menu-bar.el for whatever reason and not advertised at all.

tl;dr: In-band signalling sucks big time.