* bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)
@ 2010-02-05 2:55 Vivek Dasmohapatra
2010-02-05 4:15 ` Chong Yidong
0 siblings, 1 reply; 5+ messages in thread
From: Vivek Dasmohapatra @ 2010-02-05 2:55 UTC (permalink / raw)
To: 5526
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)
2010-02-05 2:55 bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3) Vivek Dasmohapatra
@ 2010-02-05 4:15 ` Chong Yidong
2010-02-05 14:05 ` Vivek Dasmohapatra
0 siblings, 1 reply; 5+ messages in thread
From: Chong Yidong @ 2010-02-05 4:15 UTC (permalink / raw)
To: Vivek Dasmohapatra; +Cc: 5526
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)
2010-02-05 4:15 ` Chong Yidong
@ 2010-02-05 14:05 ` Vivek Dasmohapatra
2010-02-06 13:27 ` Chong Yidong
0 siblings, 1 reply; 5+ messages in thread
From: Vivek Dasmohapatra @ 2010-02-05 14:05 UTC (permalink / raw)
To: Chong Yidong; +Cc: 5526
[-- 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)
2010-02-05 14:05 ` Vivek Dasmohapatra
@ 2010-02-06 13:27 ` Chong Yidong
2010-02-06 18:08 ` Vivek Dasmohapatra
0 siblings, 1 reply; 5+ messages in thread
From: Chong Yidong @ 2010-02-06 13:27 UTC (permalink / raw)
To: Vivek Dasmohapatra; +Cc: 5526
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.)
^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3)
2010-02-06 13:27 ` Chong Yidong
@ 2010-02-06 18:08 ` Vivek Dasmohapatra
0 siblings, 0 replies; 5+ messages in thread
From: Vivek Dasmohapatra @ 2010-02-06 18:08 UTC (permalink / raw)
To: Chong Yidong; +Cc: 5526
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-02-06 18:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-05 2:55 bug#5526: 23.1; (master) - charset.c possible unwise use of alloca(3) Vivek Dasmohapatra
2010-02-05 4:15 ` Chong Yidong
2010-02-05 14:05 ` Vivek Dasmohapatra
2010-02-06 13:27 ` Chong Yidong
2010-02-06 18:08 ` Vivek Dasmohapatra
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.