Hi, xmms2-devel! Some days ago I've wandered on cppcheck[1] utility, decided to give it a try on some projects I use to use and it eventually found some real resource leaks!
Fixing them is usually not so hard so it can be a good start in a new
project.
I hope I've properly fixed obvious ones (avahi ones need review from
more experienced people)
There is yet some frightening cppcheck reports[3], which I failed to fix.
That code seems to call for serious restructuring.
Whole set of patches is available at git repo:
git://repo.or.cz/xmms2-devel/slyfox.git cppcheck_resleaks
See patches in text/plain attach. I've already got some feedback
on them, so I don't expect to merge them as-is [2]
The question is whether you need these patches and if so: in what
way I should restructure/send them?
Thanks!
[1]
http://apps.sourceforge.net/trac/cppcheck/
Run as
$ cppcheck .
or
$ cppcheck --all --force .
for paranoid mode.
[2] on #xmms2
00:24:58 < _caotic_> That bool variable that get's malloced and freed in the
daap plugin is already broken. That it might not get freed correctly is a minor
problem in that code.
00:25:57 < trofi> yes, cppcheck complains a lot in daap code
00:26:10 < trofi> i gave up trying to do something with it
00:26:56 < Gibheer> CFLAGS does not change anything
00:27:24 < trofi> it seems to pass that gmalloc'ed bool to avahi priv state
00:27:32 < trofi> so, might be not so broken
00:30:15 < _caotic_> trofi: I think it would be better to use two different
callback functions instead of passing true or
false to one common callback functions
[3]
[./src/plugins/daap/daap_xform.c:221]: (error) Memory leak: login_data
[./src/plugins/daap/daap_xform.c:298]: (error) Memory leak: data
[./src/plugins/daap/daap_xform.c:315]: (error) Memory leak: login_data
--
Sergei
commit 04d6e480cb35cdfd0de3d791d5e321ada37b0c77
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Tue Jan 12 00:04:04 2010 +0200
OTHED: sid: cppcheck: [./src/plugins/sid/sidplay_wrapper.cpp:184]:
(possible error) Memory leak: rs
Fixed ReSIDBuilder object leak in error paths.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/plugins/sid/sidplay_wrapper.cpp
b/src/plugins/sid/sidplay_wrapper.cpp
index 2f1c74a..87d6649 100644
--- a/src/plugins/sid/sidplay_wrapper.cpp
+++ b/src/plugins/sid/sidplay_wrapper.cpp
@@ -139,75 +139,79 @@ sidplay_wrapper_set_subtune (struct sidplay_wrapper
*wrap, gint subtune)
wrap->player->load (wrap->currTune);
}
gint
sidplay_wrapper_load (struct sidplay_wrapper *wrap, const void *buf, gint len)
{
int res;
wrap->currTune = new SidTuneMD5 (0);
res = wrap->currTune->read ((const uint_least8_t *)buf, len);
if (!res) {
return -2;
}
wrap->currTune->selectSong (1);
res = wrap->player->load (wrap->currTune);
if (res) {
return -3;
}
wrap->currTune->GetMD5 (wrap->md5);
ReSIDBuilder *rs = new ReSIDBuilder ("ReSID");
if (!rs) {
return -4;
}
if (!*rs) {
+ delete rs;
return -5;
}
rs->create ((wrap->player->info ()).maxsids);
if (!*rs) {
+ delete rs;
return -6;
}
rs->filter (false);
if (!*rs) {
+ delete rs;
return -6;
}
rs->sampling (44100);
if (!*rs) {
+ delete rs;
return -6;
}
wrap->conf = wrap->player->config ();
wrap->conf.frequency = 44100;
wrap->conf.precision = 16;
wrap->conf.playback = sid2_stereo;
wrap->conf.sampleFormat = SID2_LITTLE_SIGNED;
/* These should be configurable ... */
wrap->conf.clockSpeed = SID2_CLOCK_CORRECT;
wrap->conf.clockForced = true;
wrap->conf.sidModel = SID2_MODEL_CORRECT;
wrap->conf.optimisation = SID2_DEFAULT_OPTIMISATION;
wrap->conf.sidSamples = true;
wrap->conf.clockDefault = SID2_CLOCK_PAL;
wrap->conf.sidDefault = SID2_MOS6581;
wrap->conf.sidEmulation = rs;
res = wrap->player->config (wrap->conf);
return res;
}
const char *
sidplay_wrapper_error (struct sidplay_wrapper *wrap)
{
return wrap->player->error ();
}
commit 614466be74d0d1f9f983495239e9b3e3dd0e5d78
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Tue Jan 12 00:00:31 2010 +0200
OTHER: daap_mdns_avahi: cppcheck resource leak fixes:
[./src/plugins/daap/daap_mdns_avahi.c:123]: (possible error) Memory leak:
server
[./src/plugins/daap/daap_mdns_avahi.c:142]: (error) Memory leak: b
Moved memory allocation/deallocation away from leaky paths.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/plugins/daap/daap_mdns_avahi.c
b/src/plugins/daap/daap_mdns_avahi.c
index 7549434..9d2c77f 100644
--- a/src/plugins/daap/daap_mdns_avahi.c
+++ b/src/plugins/daap/daap_mdns_avahi.c
@@ -64,111 +64,113 @@ daap_mdns_serv_remove (GSList *serv_list, gchar *addr,
guint port)
}
}
return NULL;
}
static void
daap_mdns_resolve_cb (AvahiServiceResolver *resolv,
AvahiIfIndex iface,
AvahiProtocol proto,
AvahiResolverEvent event,
const gchar *name,
const gchar *type,
const gchar *domain,
const gchar *hostname,
const AvahiAddress *addr,
guint16 port,
AvahiStringList *text,
AvahiLookupResultFlags flags,
void *userdata)
{
gboolean *remove = userdata;
gchar ad[ADDR_LEN];
daap_mdns_server_t *server;
if (!resolv) {
return;
}
switch (event) {
case AVAHI_RESOLVER_FOUND:
- server = g_new0 (daap_mdns_server_t, 1);
avahi_address_snprint (ad, sizeof (ad), addr);
- server->server_name = g_strdup (name);
- server->address = g_strdup (ad);
- server->mdns_hostname = g_strdup (hostname);
- server->port = port;
-
if (*remove) {
g_static_mutex_lock (&serv_list_mut);
g_server_list = daap_mdns_serv_remove
(g_server_list, ad, port);
g_static_mutex_unlock (&serv_list_mut);
} else {
+ server = g_new0 (daap_mdns_server_t, 1);
+ server->server_name = g_strdup (name);
+ server->address = g_strdup (ad);
+ server->mdns_hostname = g_strdup (hostname);
+ server->port = port;
+
g_static_mutex_lock (&serv_list_mut);
g_server_list = g_slist_prepend (g_server_list,
server);
g_static_mutex_unlock (&serv_list_mut);
}
g_free (remove);
break;
case AVAHI_RESOLVER_FAILURE:
break;
default:
break;
}
avahi_service_resolver_free (resolv);
}
static void
daap_mdns_browse_cb (AvahiServiceBrowser *browser,
AvahiIfIndex iface,
AvahiProtocol proto,
AvahiBrowserEvent event,
const gchar *name,
const gchar *type,
const gchar *domain,
AvahiLookupResultFlags flags,
void *userdata)
{
gboolean ok = FALSE;
- gboolean *b = g_malloc (sizeof (gboolean));
+ gboolean *b;
AvahiClient *client = ((browse_callback_userdata_t *) userdata)->client;
if (!browser) {
return;
}
+ b = g_malloc (sizeof (gboolean));
+
switch (event) {
case AVAHI_BROWSER_NEW:
*b = FALSE;
ok = (gboolean)
avahi_service_resolver_new (client, iface, proto,
name, type,
domain,
AVAHI_PROTO_UNSPEC, 0,
daap_mdns_resolve_cb,
b);
break;
case AVAHI_BROWSER_REMOVE:
*b = TRUE;
ok = (gboolean)
avahi_service_resolver_new (client, iface, proto,
name, type,
domain,
AVAHI_PROTO_UNSPEC, 0,
daap_mdns_resolve_cb,
b);
break;
case AVAHI_BROWSER_CACHE_EXHAUSTED:
break;
case AVAHI_BROWSER_ALL_FOR_NOW:
break;
default:
break;
}
}
static void
daap_mdns_client_cb (AvahiClient *client,
commit e828977493286f4741252b8ecfa077e6738a6580
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 23:52:54 2010 +0200
OTHER: ices: cppcheck: [./src/plugins/ices/encode.c:130]: (error) Memory
leak: s
Fixed memleak on error case (nom_br <= 0).
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/plugins/ices/encode.c b/src/plugins/ices/encode.c
index f8d9085..7fe50e1 100644
--- a/src/plugins/ices/encode.c
+++ b/src/plugins/ices/encode.c
@@ -96,66 +96,68 @@ xmms_ices_encoder_create (encoder_state *s, vorbis_comment
*vc)
ogg_stream_packetin (&s->os, &header[2]);
s->in_header = TRUE;
s->flushing = FALSE;
s->samples_in_current_page = 0;
s->previous_granulepos = 0;
s->encoder_inited = TRUE;
return TRUE;
}
/* Free the ogg and vorbis encoder state associated with
* encoder_state, if the encoder is present.
*/
static void
xmms_ices_encoder_free (encoder_state *s)
{
if (s->encoder_inited) {
ogg_stream_clear (&s->os);
vorbis_block_clear (&s->vb);
vorbis_dsp_clear (&s->vd);
vorbis_info_clear (&s->vi);
s->encoder_inited = FALSE;
}
}
encoder_state *
xmms_ices_encoder_init (int min_br, int nom_br, int max_br)
{
- encoder_state *s = g_new0 (encoder_state, 1);
+ encoder_state *s;
/* If none of these are set, it's obviously not supposed to be managed
*/
if (nom_br <= 0)
return NULL;
+ s = g_new0 (encoder_state, 1);
+
s->min_br = min_br;
s->nom_br = nom_br;
s->max_br = max_br;
s->serial = 0;
s->in_header = FALSE;
s->encoder_inited = FALSE;
return s;
}
void xmms_ices_encoder_fini (encoder_state *s) {
xmms_ices_encoder_free (s);
g_free (s);
}
/* Start a new logical ogg stream. */
gboolean xmms_ices_encoder_stream_change (encoder_state *s, int rate,
int channels, vorbis_comment *vc)
{
xmms_ices_encoder_free (s);
s->rate = rate;
s->channels = channels;
return xmms_ices_encoder_create (s, vc);
}
/* Encode the given data into Ogg Vorbis. */
void xmms_ices_encoder_input (encoder_state *s, xmms_samplefloat_t *buf, int
bytes)
{
float **buffer;
int i,j;
commit c8185a480be1aa83fe73156674bbf09e3c29bd34
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 23:37:35 2010 +0200
OTHER: mdns-avahi: [./src/clients/mdns/avahi/mdns-avahi.c:117]: (error)
Deallocating a deallocated pointer: name
Here we try to fix a bunch of errors:
* we are trying to g_free statically allocated data on uname(2) failure
* we are assuming g_main_loop_quit is a noreturn function and thus
g_free'ing 'name' more, than once
* we don't free avahi group on error
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/clients/mdns/avahi/mdns-avahi.c
b/src/clients/mdns/avahi/mdns-avahi.c
index 45059ed..9dd5ce8 100644
--- a/src/clients/mdns/avahi/mdns-avahi.c
+++ b/src/clients/mdns/avahi/mdns-avahi.c
@@ -68,87 +68,93 @@ group_callback (AvahiEntryGroup *g,
switch (state) {
case AVAHI_ENTRY_GROUP_ESTABLISHED:
printf ("_xmms2._tcp port: %d is registered\n", port);
break;
case AVAHI_ENTRY_GROUP_COLLISION:
printf ("Got group collsion... terminating...\n");
g_main_loop_quit (ml);
break;
case AVAHI_ENTRY_GROUP_FAILURE:
g_main_loop_quit (ml);
break;
case AVAHI_ENTRY_GROUP_UNCOMMITED:
case AVAHI_ENTRY_GROUP_REGISTERING:
;
}
}
static void
create_services (AvahiClient *c)
{
int ret;
struct utsname uts;
gchar *name;
g_return_if_fail (c);
if (uname (&uts) != 0) {
printf ("failed to run uname()\n");
- name = "xmms2";
+ name = g_strdup ("xmms2");
} else {
name = g_strdup (uts.nodename);
}
if (!group) {
if (!(group = avahi_entry_group_new (c, group_callback, NULL)))
{
printf ("couldn't create new group!\n");
+ g_free (name);
g_main_loop_quit (ml);
+ return;
}
}
if ((ret = avahi_entry_group_add_service (group, AVAHI_IF_UNSPEC,
AVAHI_PROTO_UNSPEC,
0, name, "_xmms2._tcp",
NULL, NULL, port, NULL)) < 0)
{
printf ("couldn't add entry to group: %s\n", avahi_strerror
(ret));
g_free (name);
g_main_loop_quit (ml);
+ avahi_entry_group_free (group);
+ return;
}
g_free (name);
if ((ret = avahi_entry_group_commit (group)) < 0) {
printf ("couldn't commit group: %s\n", avahi_strerror (ret));
g_main_loop_quit (ml);
+ avahi_entry_group_free (group);
+ return;
}
return;
}
static void
client_callback (AvahiClient *c,
AvahiClientState state,
void *userdata)
{
g_return_if_fail (c);
switch (state) {
case AVAHI_CLIENT_S_RUNNING:
if (!group) {
create_services (c);
}
break;
case AVAHI_CLIENT_S_COLLISION:
printf ("Client collision, terminating...");
g_main_loop_quit (ml);
break;
case AVAHI_CLIENT_FAILURE:
printf ("Client failure: %s\n", avahi_strerror
(avahi_client_errno (c)));
g_main_loop_quit (ml);
break;
case AVAHI_CLIENT_CONNECTING:
case AVAHI_CLIENT_S_REGISTERING:
;
}
commit 5df600f7991538b17c9845ca158153616b29ec5d
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 23:21:27 2010 +0200
OTHER: cppcheck: [./src/clients/nycli/commands.c:925]: (error) Memory leak:
p
Handled one more deallocation properly in error case.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/clients/nycli/commands.c b/src/clients/nycli/commands.c
index 14faeec..4d1e564 100644
--- a/src/clients/nycli/commands.c
+++ b/src/clients/nycli/commands.c
@@ -895,60 +895,61 @@ cmd_flag_pos_get (cli_infos_t *infos, command_context_t
*ctx, gint *pos)
/* No flag given, just enqueue */
*pos = infos->cache->active_playlist->len;
}
return TRUE;
}
/* Check if url is a dir, used if matching_browse arg isn't a pattern
(should have a better way) */
static gint
url_isdir (cli_infos_t *infos, const gchar *const url)
{
xmmsc_result_t *res;
xmmsv_t *val, *entry;
gchar *p, *path, *urls = NULL, *fullurl, *scheme;
const gchar *cpath;
gint ret = 0;
p = g_strdup (url);
if ((urls = strstr (p, "://"))) {
urls += 3;
} else {
/* Expects a scheme */
g_free (p);
return 0;
}
ret = strlen (urls);
if (ret && urls[ret - 1] == '/') {
+ g_free (p);
return 1;
}
ret = 0;
for (path = urls + strlen (urls) - 1; path != urls && *path == '/';
--path) {
*path = '\0';
}
scheme = g_strndup (p, urls - p);
/* g_path_get_dirname has no notion of URLs, so
* split the scheme part and work with the filename
* part, remove the basename then concatenate the
* scheme and path part back together
*/
path = g_path_get_dirname (urls);
fullurl = g_strconcat (scheme, urls, NULL);
urls = g_strconcat (scheme, path, NULL);
g_free (path);
g_free (scheme);
g_free (p);
res = xmmsc_xform_media_browse_encoded (infos->sync, urls);
xmmsc_result_wait (res);
val = xmmsc_result_get_value (res);
if (!xmmsv_is_error (val)) {
xmmsv_list_iter_t *it;
xmmsv_get_list_iter (val, &it);
for (xmmsv_list_iter_first (it);
xmmsv_list_iter_valid (it);
commit ceeb252e49cb6d7dc0c28234af4d67b39d5321c5
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 23:14:06 2010 +0200
[./src/xmms/streamtype.c:118]: (error) Memory leak: val
val is allocated in xmms_stream_type_parse and is not
freed in error case.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/xmms/streamtype.c b/src/xmms/streamtype.c
index c499e11..b91caf2 100644
--- a/src/xmms/streamtype.c
+++ b/src/xmms/streamtype.c
@@ -88,60 +88,61 @@ xmms_stream_type_parse (va_list ap)
if (key == XMMS_STREAM_TYPE_END)
break;
if (key == XMMS_STREAM_TYPE_NAME) {
res->name = g_strdup (va_arg (ap, char *));
continue;
}
if (key == XMMS_STREAM_TYPE_PRIORITY) {
res->priority = va_arg (ap, int);
continue;
}
val = g_new0 (xmms_stream_type_val_t, 1);
val->key = key;
switch (val->key) {
case XMMS_STREAM_TYPE_MIMETYPE:
case XMMS_STREAM_TYPE_URL:
val->type = STRING;
val->d.string = g_strdup (va_arg (ap, char *));
break;
case XMMS_STREAM_TYPE_FMT_FORMAT:
case XMMS_STREAM_TYPE_FMT_CHANNELS:
case XMMS_STREAM_TYPE_FMT_SAMPLERATE:
val->type = INT;
val->d.num = va_arg (ap, int);
break;
default:
XMMS_DBG ("UNKNOWN TYPE!!");
+ g_free (val);
return NULL;
}
res->list = g_list_append (res->list, val);
}
if (!res->name) {
const gchar *mime = xmms_stream_type_get_str (res,
XMMS_STREAM_TYPE_MIMETYPE);
const gchar *url = xmms_stream_type_get_str (res,
XMMS_STREAM_TYPE_URL);
if (mime && url) {
res->name = g_strconcat (mime, ":", url, NULL);
} else if (mime) {
res->name = g_strdup (mime);
} else {
g_assert_not_reached ();
}
g_strdelimit (res->name, ".", '_');
}
if (res->priority < 0) {
res->priority = XMMS_STREAM_TYPE_PRIORITY_DEFAULT;
}
return res;
}
const char *
xmms_stream_type_get_str (const xmms_stream_type_t *st, xmms_stream_type_key_t
key)
{
commit ba08c969376824b0b565cc341f2b201a3a889bef
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 22:59:46 2010 +0200
[./src/xmms/bindata.c:260]: (error) Resource leak: fp
bindata file was not closed in some subtle error cases.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/xmms/bindata.c b/src/xmms/bindata.c
index 30cafec..90f5877 100644
--- a/src/xmms/bindata.c
+++ b/src/xmms/bindata.c
@@ -230,60 +230,61 @@ static xmmsv_t *
xmms_bindata_client_retrieve (xmms_bindata_t *bindata, const gchar *hash,
xmms_error_t *err)
{
xmmsv_t *res;
gchar *path;
GString *str;
FILE *fp;
path = xmms_bindata_build_path (bindata, hash);
fp = fopen (path, "rb");
if (!fp) {
xmms_log_error ("Requesting '%s' which is not on the server",
hash);
xmms_error_set (err, XMMS_ERROR_NOENT, "File not found!");
g_free (path);
return NULL;
}
g_free (path);
str = g_string_new (NULL);
while (!feof (fp)) {
gchar buf[1024];
gint l;
l = fread (buf, 1, 1024, fp);
if (ferror (fp)) {
g_string_free (str, TRUE);
xmms_log_error ("Error reading bindata '%s'", hash);
xmms_error_set (err, XMMS_ERROR_GENERIC, "Error reading
file");
+ fclose (fp);
return NULL;
}
g_string_append_len (str, buf, l);
}
fclose (fp);
res = xmmsv_new_bin ((unsigned char *)str->str, str->len);
g_string_free (str, TRUE);
return res;
}
static void
xmms_bindata_client_remove (xmms_bindata_t *bindata, const gchar *hash,
xmms_error_t *err)
{
gchar *path;
path = xmms_bindata_build_path (bindata, hash);
if (unlink (path) == -1) {
xmms_error_set (err, XMMS_ERROR_GENERIC, "Couldn't remove
file");
}
g_free (path);
return;
}
static GList *
xmms_bindata_client_list (xmms_bindata_t *bindata, xmms_error_t *err)
{
commit 9ca3e9a75797ca6e71051a6d66ced5d9f5f44f71
Author: Sergei Trofimovich <sly...@inbox.ru>
Date: Mon Jan 11 22:56:37 2010 +0200
OTHER: cppcheck: [./src/clients/cli/main.c:204]: (possible error) Resource
leak: fp
Config file was not freed after reading and parsing it.
Thanks to cppcheck.
Signed-off-by: Sergei Trofimovich <sly...@inbox.ru>
diff --git a/src/clients/cli/main.c b/src/clients/cli/main.c
index fc09a5a..477b113 100644
--- a/src/clients/cli/main.c
+++ b/src/clients/cli/main.c
@@ -165,60 +165,61 @@ read_config (void)
fclose (fp);
} else {
print_info ("Could not create configfile: %s\nMake sure
you have write permissions to that location.", file);
}
}
if (g_file_test (file, G_FILE_TEST_EXISTS)) {
fp = fopen (file, "r");
if (!fp) {
print_error ("Could not open configfile %s", file);
}
g_free (file);
if (fstat (fileno (fp), &st) == -1) {
print_error ("fstat");
}
buffer = g_malloc0 (st.st_size + 1);
while (read_bytes < st.st_size) {
guint ret = fread (buffer + read_bytes,
st.st_size - read_bytes, 1, fp);
if (ret == 0) {
break;
}
read_bytes += ret;
g_assert (read_bytes >= 0);
}
+ fclose (fp);
config = parse_config (buffer);
g_free (buffer);
} else {
g_free (file);
config = parse_config (defaultconfig);
}
return config;
}
static void
free_config (void)
{
if (config) {
g_hash_table_destroy (config);
}
}
/**
* Usage
*/
static void
cmd_help (xmmsc_connection_t *conn, gint argc, gchar **argv)
{
gint i;
signature.asc
Description: PGP signature
-- _______________________________________________ Xmms2-devel mailing list Xmms2-devel@lists.xmms.se http://lists.xmms.se/cgi-bin/mailman/listinfo/xmms2-devel
