Am 14.09.2012 00:38 schrieb Stefan Tauner:
In dmi_init() we populate static char *dmistrings[] with values that get later compared in dmi_match(). Those strings are actually strduped in get_dmi_string() and hence need to be freed later. This patch accomplishes this by registering another shutdown method.
This bug was found thanks to valgrind.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Good find, especially the puts() which had crept in there. Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net with a small comment.
diff --git a/dmi.c b/dmi.c index dfc78e9..fd260a6 100644 --- a/dmi.c +++ b/dmi.c @@ -144,16 +144,28 @@ static char *get_dmi_string(const char *string_name)
result = strdup(answerbuf); if (!result)
puts("WARNING: Out of memory - DMI support fails");
msg_perr("WARNING: Out of memory - DMI support fails");return result;
}
+static int dmi_shutdown(void *data) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(dmistrings); i++) {
free(dmistrings[i]);
libflashrom allows calling programmer_init() and programmer_shutdown() again after one flashrom run. For that case, we want the second run to have zeroed dmistrings[i]. Please insert dmistrings[i] = NULL;
- }
- return 0;
+}
void dmi_init(void) { int i; char *chassis_type;
- if (register_shutdown(dmi_shutdown, NULL))
return;- has_dmi_support = 1; for (i = 0; i < ARRAY_SIZE(dmidecode_names); i++) { dmistrings[i] = get_dmi_string(dmidecode_names[i]);
Regards, Carl-Daniel
On Sun, 23 Sep 2012 01:30:50 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
+static int dmi_shutdown(void *data) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(dmistrings); i++) {
free(dmistrings[i]);libflashrom allows calling programmer_init() and programmer_shutdown() again after one flashrom run. For that case, we want the second run to have zeroed dmistrings[i]. Please insert dmistrings[i] = NULL;
IIRC i even had this in first. it is not necessary though because they get overwritten by dmi_init() which gets called unconditionally by internal_init(). i am not convinced, that it is a good idea to null them anyway. note that the array is not initialized at startup either.
Am 23.09.2012 13:51 schrieb Stefan Tauner:
On Sun, 23 Sep 2012 01:30:50 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
+static int dmi_shutdown(void *data) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(dmistrings); i++) {
free(dmistrings[i]);libflashrom allows calling programmer_init() and programmer_shutdown() again after one flashrom run. For that case, we want the second run to have zeroed dmistrings[i]. Please insert dmistrings[i] = NULL;
IIRC i even had this in first. it is not necessary though because they get overwritten by dmi_init() which gets called unconditionally by internal_init().
Yes, I saw that code too. I am just a bit uncomfortable with leaving invalid non-null pointers around.
i am not convinced, that it is a good idea to null them anyway. note that the array is not initialized at startup either.
I'm not happy about uninitialized static arrays. That said, I think we could kill a few globals if there was a struct with private programmer data, containing DMI strings, coreboot names, and other fun stuff. That way we could initialize everything in one fell swoop.
Regards, Carl-Daniel
On Sun, 23 Sep 2012 14:00:43 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 23.09.2012 13:51 schrieb Stefan Tauner:
On Sun, 23 Sep 2012 01:30:50 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
+static int dmi_shutdown(void *data) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(dmistrings); i++) {
free(dmistrings[i]);libflashrom allows calling programmer_init() and programmer_shutdown() again after one flashrom run. For that case, we want the second run to have zeroed dmistrings[i]. Please insert dmistrings[i] = NULL;
IIRC i even had this in first. it is not necessary though because they get overwritten by dmi_init() which gets called unconditionally by internal_init().
Yes, I saw that code too. I am just a bit uncomfortable with leaving invalid non-null pointers around.
me too, but i am also uncomfortable with adding redundant code. i have committed the patch including the nullifying code anyway in r1604, thanks.
i am not convinced, that it is a good idea to null them anyway. note that the array is not initialized at startup either.
I'm not happy about uninitialized static arrays. That said, I think we could kill a few globals if there was a struct with private programmer data, containing DMI strings, coreboot names, and other fun stuff. That way we could initialize everything in one fell swoop.
my guts tell me this sounds way better than it would actually work like :)
-
Carl-Daniel Hailfinger -
Stefan Tauner