bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

[Vivek Dasmohapatra]

In src/charset.c, in load_charset_map_from_file (and .._vector)
alloca is used to allocate sizeof (struct charset_map_entries) bytes:
which is 786436 bytes - this segfaults while building in a qemu-armel
scratchbox (alloca does not return an error, so its return value cannot
be checked).

In any case, this is more than MAX_ALLOCA, and can occur several times
in a while(1) loop in the same function: I've replaced the alloca with
calloc()/walk-the-linked-list-and-free() and the build seems much happier
so far - would you be interested in a patch?

bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

[Chong Yidong]

Vivek Dasmohapatra <vivek@etla.org> writes:

> In src/charset.c, in load_charset_map_from_file (and .._vector)
> alloca is used to allocate sizeof (struct charset_map_entries) bytes:
> which is 786436 bytes - this segfaults while building in a qemu-armel
> scratchbox (alloca does not return an error, so its return value cannot
> be checked).
>
> In any case, this is more than MAX_ALLOCA, and can occur several times
> in a while(1) loop in the same function.

Thanks for pointing this out.  I have changed it to use the heap
instead.

bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

[Vivek Dasmohapatra]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 346 bytes --]

Tags: patch

> Thanks for pointing this out.  I have changed it to use the heap
> instead.

There are actually 3 other locations where the same thing happens, and the
allocated blocks are stored in a linked list which you need to walk and
free (I believe) if you switch over to using xmalloc instead. The attached
patch should do the right thing.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=0001-Use-xmalloc-xfree-instead-of-alloc-to-allocate-large.patch, Size: 2749 bytes --]

From 26b03c9ffe4d0037c3139297ac8e2689b489bbb1 Mon Sep 17 00:00:00 2001
From: Vivek Dasmohapatra <vivek@collabora.co.uk>
Date: Fri, 5 Feb 2010 13:56:44 +0000
Subject: [PATCH] Use xmalloc/xfree instead of alloc to allocate large tempporary charset structs.


diff --git a/src/charset.c b/src/charset.c
index 9e8ff1d..48339c2 100644
--- a/src/charset.c
+++ b/src/charset.c
@@ -526,7 +526,8 @@ load_charset_map_from_file (charset, mapfile, control_flag)
     error ("Failure in loading charset map: %S", SDATA (mapfile));
 
   head = entries = ((struct charset_map_entries *)
-		    xmalloc (sizeof (struct charset_map_entries)));
+                    xmalloc (sizeof (struct charset_map_entries)));
+
   n_entries = 0;
   eof = 0;
   while (1)
@@ -550,9 +551,10 @@ load_charset_map_from_file (charset, mapfile, control_flag)
       if (n_entries > 0 && (n_entries % 0x10000) == 0)
 	{
 	  entries->next = ((struct charset_map_entries *)
-			   alloca (sizeof (struct charset_map_entries)));
+                           xmalloc (sizeof (struct charset_map_entries)));
 	  entries = entries->next;
 	}
+
       idx = n_entries % 0x10000;
       entries->entry[idx].from = from;
       entries->entry[idx].to = to;
@@ -563,7 +565,12 @@ load_charset_map_from_file (charset, mapfile, control_flag)
   close (fd);
 
   load_charset_map (charset, head, n_entries, control_flag);
-  xfree (head);
+
+  for (entries = head; entries; entries = head)
+    {
+      head = entries->next; /* move the head of the list on */
+      xfree (entries);      /* free the old head            */
+    }
 }
 
 static void
@@ -586,7 +593,8 @@ load_charset_map_from_vector (charset, vec, control_flag)
     }
 
   head = entries = ((struct charset_map_entries *)
-		    alloca (sizeof (struct charset_map_entries)));
+                    xmalloc (sizeof (struct charset_map_entries)));
+
   n_entries = 0;
   for (i = 0; i < len; i += 2)
     {
@@ -620,7 +628,7 @@ load_charset_map_from_vector (charset, vec, control_flag)
       if (n_entries > 0 && (n_entries % 0x10000) == 0)
 	{
 	  entries->next = ((struct charset_map_entries *)
-			   alloca (sizeof (struct charset_map_entries)));
+                           xmalloc (sizeof (struct charset_map_entries), 1));
 	  entries = entries->next;
 	}
       idx = n_entries % 0x10000;
@@ -631,6 +639,11 @@ load_charset_map_from_vector (charset, vec, control_flag)
     }
 
   load_charset_map (charset, head, n_entries, control_flag);
+  for (entries = head; entries; entries = head)
+    {
+      head = entries->next; /* move the head of the list on */
+      xfree (entries);      /* free the old head            */
+    }
 }
 
 
-- 
1.6.6.1

bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

[Chong Yidong]

Vivek Dasmohapatra <vivek@etla.org> writes:

> There are actually 3 other locations where the same thing happens, and the
> allocated blocks are stored in a linked list which you need to walk and
> free (I believe) if you switch over to using xmalloc instead. The attached
> patch should do the right thing.

Actually, after looking at this further, I think we need to use
SAFE_ALLOCA instead of xmalloc.  I've changed the code accordingly.
Thanks for pointing out the additional changes required.  (SAFE_ALLOCA
has a bit more overhead, but I don't see a performance problem after
making this change.)

bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)

[Vivek Dasmohapatra]

> Actually, after looking at this further, I think we need to use
> SAFE_ALLOCA instead of xmalloc.  I've changed the code accordingly.

Whatever you think is best... I wasn't sure if SAFE_ALLOCA was the right 
thing to use, since the size of the struct was so much > than MAX_ALLOCA
that the xmalloc path would always be taken.

I suspect it's unlikely to impact performance as I don't imagine emacs 
spends a lot of time loading charsets over and over again.