GNU bug report logs - #71929
30.0.60; crash in mark_image_cache

Previous Next

Full log

View this message in rfc822 format

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 71929 <at> debbugs.gnu.org, spwhitton <at> spwhitton.name
Subject: bug#71929: 30.0.60; crash in mark_image_cache
Date: Tue, 09 Jul 2024 23:02:22 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Po Lu <luangruo <at> yahoo.com>
>> Cc: 71929 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>
>> Date: Tue, 09 Jul 2024 22:03:34 +0800
>> 
>> OK, I believe I understand the source of these crashes.  A frame
>> whose
>> image cache is shared among several frames is destroyed, but its
>> `image_cache' field is never cleared after it is destroyed, as its
>> cache
>> continues to be referenced, and, if references to the dead frame
>> remain,
>> GC attempts to mark the said image cache although its validity is no
>> longer guaranteed.  In earlier Emacs versions, this problem would
>> have
>> appeared if references to dead frames were preserved beyond the
>> destruction of a display structure.  This has been corrected on the
>> emacs-30 branch, and therefore if the crashes do not resurface in a
>> few
>> days, I will close this ticket.
>
> Thanks, but I don't think I understand this part of the change you
> installed:
>
>   --- a/src/image.c
>   +++ b/src/image.c
>   @@ -2304,23 +2304,18 @@ uncache_image (struct frame *f, Lisp_Object spec)
>    free_image_cache (struct frame *f)
>    {
>      struct image_cache *c = FRAME_IMAGE_CACHE (f);
>   -  if (c)
>   -    {
>   -      ptrdiff_t i;
>   +  ptrdiff_t i;
>
>   -      /* Cache should not be referenced by any frame when freed.  */
>   -      eassert (c->refcount == 0);
>   +  /* Cache should not be referenced by any frame when freed.  */
>   +  eassert (c->refcount == 0);
>
>   -      for (i = 0; i < c->used; ++i)
>   -       free_image (f, c->images[i]);
>   -      xfree (c->images);
>   -      xfree (c->buckets);
>   -      xfree (c);
>   -      FRAME_IMAGE_CACHE (f) = NULL;
>   -    }
>   +  for (i = 0; i < c->used; ++i)
>   +    free_image (f, c->images[i]);
>   +  xfree (c->images);
>   +  xfree (c->buckets);
>   +  xfree (c);
>    }
>
> This basically removes the test of 'c' being non-NULL, leaving the
> rest of the code unchanged.  But if 'c' is NULL, dereferencing it in
> the following code will segfault, so why remove the test?  In
> particular, what about frames that were not yet allocated the image
> cache (could this happen with TTY frames, for example)?
>
> What am I missing here?

That free_frame_faces has been the sole caller of this function for
quite some time, and it already performs the same test around its call
to free_image_cache.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.