Build segfaults when loading image.el

Build segfaults when loading image.el

[Dhruva Krishnamurthy]

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

Loading image.el segfaults when ImageMagick is enabled on MacOS (do not
have access to other platforms) since image magic environment is not
initialized yet. The following patch on HEAD fixes the crash by
initializing it before use.

-dhruva

diff --git a/src/image.c b/src/image.c
index 70d932f9ed..50d90ccb55 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8603,6 +8603,20 @@ #define DrawRectangle DrawRectangleGif
     MagickPixelPacket *);
 #endif

+/* Initialize the ImageMagick environment if not initialized.  */
+
+static void
+imagemagick_initialize(void)
+{
+  /* Initialize the ImageMagick environment.  */
+  static bool imagemagick_initialized;
+  if (!imagemagick_initialized)
+    {
+      MagickWandGenesis();
+      imagemagick_initialized = !!IsMagickWandInstantiated();
+    }
+}
+
 /* Log ImageMagick error message.
    Useful when an ImageMagick function returns the status `MagickFalse'.
 */

@@ -8876,12 +8890,7 @@ imagemagick_load_image (struct frame *f, struct
image *img,
   char *filename_hint = NULL;

   /* Initialize the ImageMagick environment.  */
-  static bool imagemagick_initialized;
-  if (!imagemagick_initialized)
-    {
-      imagemagick_initialized = true;
-      MagickWandGenesis ();
-    }
+  imagemagick_initialize();

   /* Handle image index for image types who can contain more than one
image.
      Interface :index is same as for GIF.  First we "ping" the image to
see how
@@ -9290,6 +9299,9 @@ DEFUN ("imagemagick-types", Fimagemagick_types,
Simagemagick_types, 0, 0, 0,
   char **imtypes;
   size_t i;

+  /* Initialize the ImageMagick environment.  */
+  imagemagick_initialize();
+
   ex = AcquireExceptionInfo ();
   imtypes = GetMagickList ("*", &numf, ex);
   DestroyExceptionInfo (ex);

[-- Attachment #2: Type: text/html, Size: 3929 bytes --]

Re: Build segfaults when loading image.el

[Dhruva Krishnamurthy]

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

Missed providing other details:

# Build host env
$ uname -a
Darwin C02V341YHTD8 19.2.0 Darwin Kernel Version 19.2.0: Sat Nov  9
03:47:04 PST 2019; root:xnu-6153.61.1~20/RELEASE_X86_64 x86_64 i386
MacBookPro14,3 Darwin

# Configure options
$ configure CC=clang --with-imagemagick

# ImageMagic details
$ MagickWand-config --cflags --ldflags
-Xpreprocessor -fopenmp -DMAGICKCORE_HDRI_ENABLE=1
-DMAGICKCORE_QUANTUM_DEPTH=16 -Xpreprocessor -fopenmp
-DMAGICKCORE_HDRI_ENABLE=1 -DMAGICKCORE_QUANTUM_DEPTH=16
-I/usr/local/Cellar/imagemagick/7.0.9-10/include/ImageMagick-7
-L/usr/local/Cellar/imagemagick/7.0.9-10/lib -lMagickWand-7.Q16HDRI
-lMagickCore-7.Q16HDRI


On Mon, Dec 23, 2019 at 1:59 PM Dhruva Krishnamurthy <dhruvakm@gmail.com>
wrote:

> Loading image.el segfaults when ImageMagick is enabled on MacOS (do not
> have access to other platforms) since image magic environment is not
> initialized yet. The following patch on HEAD fixes the crash by
> initializing it before use.
>
> -dhruva
>
> diff --git a/src/image.c b/src/image.c
> index 70d932f9ed..50d90ccb55 100644
> --- a/src/image.c
> +++ b/src/image.c
> @@ -8603,6 +8603,20 @@ #define DrawRectangle DrawRectangleGif
>      MagickPixelPacket *);
>  #endif
>
> +/* Initialize the ImageMagick environment if not initialized.  */
> +
> +static void
> +imagemagick_initialize(void)
> +{
> +  /* Initialize the ImageMagick environment.  */
> +  static bool imagemagick_initialized;
> +  if (!imagemagick_initialized)
> +    {
> +      MagickWandGenesis();
> +      imagemagick_initialized = !!IsMagickWandInstantiated();
> +    }
> +}
> +
>  /* Log ImageMagick error message.
>     Useful when an ImageMagick function returns the status `MagickFalse'.
>  */
>
> @@ -8876,12 +8890,7 @@ imagemagick_load_image (struct frame *f, struct
> image *img,
>    char *filename_hint = NULL;
>
>    /* Initialize the ImageMagick environment.  */
> -  static bool imagemagick_initialized;
> -  if (!imagemagick_initialized)
> -    {
> -      imagemagick_initialized = true;
> -      MagickWandGenesis ();
> -    }
> +  imagemagick_initialize();
>
>    /* Handle image index for image types who can contain more than one
> image.
>       Interface :index is same as for GIF.  First we "ping" the image to
> see how
> @@ -9290,6 +9299,9 @@ DEFUN ("imagemagick-types", Fimagemagick_types,
> Simagemagick_types, 0, 0, 0,
>    char **imtypes;
>    size_t i;
>
> +  /* Initialize the ImageMagick environment.  */
> +  imagemagick_initialize();
> +
>    ex = AcquireExceptionInfo ();
>    imtypes = GetMagickList ("*", &numf, ex);
>    DestroyExceptionInfo (ex);
>
>
>

[-- Attachment #2: Type: text/html, Size: 5912 bytes --]

Re: Build segfaults when loading image.el

[Paul Eggert]

Thanks, I installed that onto the emacs-27 branch on Savannah.

Re: Build segfaults when loading image.el

[Dhruva Krishnamurthy]

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

I still feel the original patch calling into IsMagickWandInstantiated() was
more correct since we were fetching it from ImageMagick library and not
setting it to true without checking if it is really initialized.

-dhruva

On Mon, Dec 23, 2019 at 5:24 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> Thanks, I installed that onto the emacs-27 branch on Savannah.
>

[-- Attachment #2: Type: text/html, Size: 969 bytes --]

Re: Build segfaults when loading image.el

[Paul Eggert]

On 12/23/19 6:09 PM, Dhruva Krishnamurthy wrote:
> I still feel the original patch calling into IsMagickWandInstantiated() was
> more correct 

Not sure I follow, but anyway any change along those lines would be more
experimental, whereas the emacs-27 branch is for bug-fixing and is maintained
more conservatively.

PS. I didn't notice the "!!IsMagickWandInstantiated();" part of your patch; if I
had, I would have omitted it from the emacs-27 branch and would have said why.

Re: Build segfaults when loading image.el

[Dhruva Krishnamurthy]

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

On further reading the ImageMagick source, I would that the calls to
'MagickWandGenesis' protect themselves against redundant repeat calls.
Hence, there is no need to protect it using a static variable. The
following patch is a minimal fix.

Link to source:
https://github.com/ImageMagick/ImageMagick/blob/f775a5cf27a95c42bb6d19b50f4869db265fdaa9/MagickWand/magick-wand.c#L1001

-dhruva

commit 23236f26d2f5839f2fdb7022039dcf102dcf3a3b
Author: Dhruva Krishnamurthy <dhruvakm@gmail.com>
Date:   Mon Dec 23 14:49:51 2019 -0800

    Initialize ImageMagick runtime environment before use

diff --git a/src/image.c b/src/image.c
index 70d932f9ed..75c31cad57 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8875,13 +8875,8 @@ imagemagick_load_image (struct frame *f, struct
image *img,
   char hint_buffer[MaxTextExtent];
   char *filename_hint = NULL;

-  /* Initialize the ImageMagick environment.  */
-  static bool imagemagick_initialized;
-  if (!imagemagick_initialized)
-    {
-      imagemagick_initialized = true;
-      MagickWandGenesis ();
-    }
+  /* Initialize the ImageMagick environment if not initialized. */
+  MagickWandGenesis ();

   /* Handle image index for image types who can contain more than one
image.
      Interface :index is same as for GIF.  First we "ping" the image to
see how
@@ -9290,6 +9285,9 @@ DEFUN ("imagemagick-types", Fimagemagick_types,
Simagemagick_types, 0, 0, 0,
   char **imtypes;
   size_t i;

+  /* Initialize the ImageMagick environment if not initialized. */
+  MagickWandGenesis ();
+
   ex = AcquireExceptionInfo ();
   imtypes = GetMagickList ("*", &numf, ex);
   DestroyExceptionInfo (ex);


On Mon, Dec 23, 2019 at 6:51 PM Paul Eggert <eggert@cs.ucla.edu> wrote:

> On 12/23/19 6:09 PM, Dhruva Krishnamurthy wrote:
> > I still feel the original patch calling into IsMagickWandInstantiated()
> was
> > more correct
>
> Not sure I follow, but anyway any change along those lines would be more
> experimental, whereas the emacs-27 branch is for bug-fixing and is
> maintained
> more conservatively.
>
> PS. I didn't notice the "!!IsMagickWandInstantiated();" part of your
> patch; if I
> had, I would have omitted it from the emacs-27 branch and would have said
> why.
>

[-- Attachment #2: Type: text/html, Size: 4603 bytes --]

Re: Build segfaults when loading image.el

[VanL]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Thanks, I installed that onto the emacs-27 branch on Savannah.

Off-topic, what are all the packages needed to build emacs-27 on Debian
10 Buster?  I could only find [1] from 'apt-get install' in the archive
search.





Footnotes: 
[1]  https://lists.gnu.org/archive/html/emacs-devel/2016-08/msg00689.html

-- 
 əə0@ 🐞 7 6 4 5
 一 二 三 言 語 𝔖 元 示 証 明 海 自 己 漢 本 人 Gnus/Emacs (berkeley-unix)

Re: Build segfaults when loading image.el

[Lars Ingebrigtsen]

VanL <van@scratch.space> writes:

> Off-topic, what are all the packages needed to build emacs-27 on Debian
> 10 Buster?  I could only find [1] from 'apt-get install' in the archive
> search.

# apt build-dep emacs

should install what you need.

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

Re: Build segfaults when loading image.el

[VanL]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> # apt build-dep emacs
>
> should install what you need.

Thanks, a deb-src line is required in /etc/apt/sources.list also.

-- 
 əə0@ 7 6 4 5 bit byte word 6 5 0 2 memory map dma intelligence io 🐞
 一 二 三 言 語 𝔖 元 示 証 明 海 自 己 漢 本 人 Gnus/Emacs (berkeley-unix)