From f01d7d230b1835ab035a5130e2a39f05f0aabdf5 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 18:59:03 +0100 Subject: [PATCH 1/7] input/file: don't fall back to parent directory This code has never made any sense, and has broken some of the archive plugin. --- NEWS | 2 ++ src/input/file_input_plugin.c | 21 +++------------------ 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/NEWS b/NEWS index b375eefd7..5ef6e21d2 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ ver 0.15.7 (2009/??/??) +* input: + - file: don't fall back to parent directory * tags: - id3: fix ID3v1 charset conversion * decoders: diff --git a/src/input/file_input_plugin.c b/src/input/file_input_plugin.c index 64a4030ab..bda1777ac 100644 --- a/src/input/file_input_plugin.c +++ b/src/input/file_input_plugin.c @@ -36,25 +36,14 @@ input_file_open(struct input_stream *is, const char *filename) int fd, ret; struct stat st; - char* pathname = g_strdup(filename); - if (filename[0] != '/') - { - g_free(pathname); return false; - } - if (stat(filename, &st) < 0) { - char* slash = strrchr(pathname, '/'); - *slash = '\0'; - } - - fd = open(pathname, O_RDONLY); + fd = open(filename, O_RDONLY); if (fd < 0) { is->error = errno; g_debug("Failed to open \"%s\": %s", - pathname, g_strerror(errno)); - g_free(pathname); + filename, g_strerror(errno)); return false; } @@ -64,15 +53,13 @@ input_file_open(struct input_stream *is, const char *filename) if (ret < 0) { is->error = errno; close(fd); - g_free(pathname); return false; } if (!S_ISREG(st.st_mode)) { - g_debug("Not a regular file: %s", pathname); + g_debug("Not a regular file: %s", filename); is->error = EINVAL; close(fd); - g_free(pathname); return false; } @@ -86,8 +73,6 @@ input_file_open(struct input_stream *is, const char *filename) is->data = GINT_TO_POINTER(fd); is->ready = true; - g_free(pathname); - return true; } From 83aac2a057069de88e551899583fa0ab5662f753 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:07:20 +0100 Subject: [PATCH 2/7] test/run_input: initialize archive plugins Enable archive plugin debugging. --- test/run_input.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/run_input.c b/test/run_input.c index 37c792b86..b91d367ba 100644 --- a/test/run_input.c +++ b/test/run_input.c @@ -17,11 +17,16 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include "config.h" #include "input_stream.h" #include "tag_pool.h" #include "tag_save.h" #include "conf.h" +#ifdef ENABLE_ARCHIVE +#include "archive_list.h" +#endif + #include #include @@ -58,6 +63,11 @@ int main(int argc, char **argv) tag_pool_init(); config_global_init(); + +#ifdef ENABLE_ARCHIVE + archive_plugin_init_all(); +#endif + input_stream_global_init(); /* open the stream and wait until it becomes ready */ @@ -107,6 +117,11 @@ int main(int argc, char **argv) input_stream_close(&is); input_stream_global_finish(); + +#ifdef ENABLE_ARCHIVE + archive_plugin_deinit_all(); +#endif + config_global_finish(); tag_pool_deinit(); From 81aa58efa8912f8c042650d82e9c3be155f343bc Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:36:22 +0100 Subject: [PATCH 3/7] test/run_input: deinitialize everything after open() error This enables valgrind debugging after an error occurred. --- test/run_input.c | 98 ++++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 44 deletions(-) diff --git a/test/run_input.c b/test/run_input.c index b91d367ba..5d74473e3 100644 --- a/test/run_input.c +++ b/test/run_input.c @@ -41,14 +41,58 @@ my_log_func(const gchar *log_domain, G_GNUC_UNUSED GLogLevelFlags log_level, g_printerr("%s\n", message); } -int main(int argc, char **argv) +static int +dump_input_stream(struct input_stream *is) { - struct input_stream is; - bool success; char buffer[4096]; size_t num_read; ssize_t num_written; + /* wait until the stream becomes ready */ + + while (!is->ready) { + int ret = input_stream_buffer(is); + if (ret < 0) + /* error */ + return 2; + + if (ret == 0) + /* nothing was buffered - wait */ + g_usleep(10000); + } + + /* print meta data */ + + if (is->mime != NULL) + g_printerr("MIME type: %s\n", is->mime); + + /* read data and tags from the stream */ + + while (!input_stream_eof(is)) { + struct tag *tag = input_stream_tag(is); + if (tag != NULL) { + g_printerr("Received a tag:\n"); + tag_save(stderr, tag); + tag_free(tag); + } + + num_read = input_stream_read(is, buffer, sizeof(buffer)); + if (num_read == 0) + break; + + num_written = write(1, buffer, num_read); + if (num_written <= 0) + break; + } + + return 0; +} + +int main(int argc, char **argv) +{ + struct input_stream is; + int ret; + if (argc != 2) { g_printerr("Usage: run_input URI\n"); return 1; @@ -70,52 +114,18 @@ int main(int argc, char **argv) input_stream_global_init(); - /* open the stream and wait until it becomes ready */ + /* open the stream and dump it */ - success = input_stream_open(&is, argv[1]); - if (!success) { + if (input_stream_open(&is, argv[1])) { + ret = dump_input_stream(&is); + input_stream_close(&is); + } else { g_printerr("input_stream_open() failed\n"); - return 2; - } - - while (!is.ready) { - int ret = input_stream_buffer(&is); - if (ret < 0) - /* error */ - return 2; - - if (ret == 0) - /* nothing was buffered - wait */ - g_usleep(10000); - } - - /* print meta data */ - - if (is.mime != NULL) - g_printerr("MIME type: %s\n", is.mime); - - /* read data and tags from the stream */ - - while (!input_stream_eof(&is)) { - struct tag *tag = input_stream_tag(&is); - if (tag != NULL) { - g_printerr("Received a tag:\n"); - tag_save(stderr, tag); - tag_free(tag); - } - - num_read = input_stream_read(&is, buffer, sizeof(buffer)); - if (num_read == 0) - break; - - num_written = write(1, buffer, num_read); - if (num_written <= 0) - break; + ret = 2; } /* deinitialize everything */ - input_stream_close(&is); input_stream_global_finish(); #ifdef ENABLE_ARCHIVE @@ -125,5 +135,5 @@ int main(int argc, char **argv) config_global_finish(); tag_pool_deinit(); - return 0; + return ret; } From 2234d491b7c1a8cbea0cb4e444fc72c37aec5016 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:31:50 +0100 Subject: [PATCH 4/7] input/archive: close the archive file on error Fixed memory leak in error handler. --- NEWS | 1 + src/input/archive_input_plugin.c | 1 + 2 files changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 5ef6e21d2..6bcf328f8 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,7 @@ ver 0.15.7 (2009/??/??) * input: - file: don't fall back to parent directory + - archive: fixed memory leak in error handler * tags: - id3: fix ID3v1 charset conversion * decoders: diff --git a/src/input/archive_input_plugin.c b/src/input/archive_input_plugin.c index 6239f4298..8e897f0c2 100644 --- a/src/input/archive_input_plugin.c +++ b/src/input/archive_input_plugin.c @@ -66,6 +66,7 @@ input_archive_open(struct input_stream *is, const char *pathname) if (!opened) { g_warning("open inarchive file %s failed\n\n",filename); + arplug->close(file); } else { is->ready = true; } From 6c0f50efb568a6da52e6c31920bc93c4254063c7 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:42:54 +0100 Subject: [PATCH 5/7] archive/bz2: removed NULL check before g_free() g_free(NULL) is allowed. --- src/archive/bz2_plugin.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/archive/bz2_plugin.c b/src/archive/bz2_plugin.c index 4db68f48e..78f13400a 100644 --- a/src/archive/bz2_plugin.c +++ b/src/archive/bz2_plugin.c @@ -140,8 +140,8 @@ static void bz2_close(struct archive_file *file) { bz2_context *context = (bz2_context *) file; - if (context->name) - g_free(context->name); + + g_free(context->name); input_stream_close(&context->istream); g_free(context); From 3411f6cffdcf3c72e7cee3a263c40414dfef956e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:45:50 +0100 Subject: [PATCH 6/7] archive: close archive when stream is closed Fixes a memory leak: the "archive" input plugin opens the archive, but never closes it. This patch moves the responsibility for doing that to archive_plugin.open_stream(). This is an slight internal API change, but it is the simplest and least intrusive fix for the memory leak. --- NEWS | 2 ++ src/archive/bz2_plugin.c | 2 ++ src/archive/iso_plugin.c | 2 ++ src/archive/zip_plugin.c | 2 ++ src/archive_api.h | 3 +++ 5 files changed, 11 insertions(+) diff --git a/NEWS b/NEWS index 6bcf328f8..7ead1f16e 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ ver 0.15.7 (2009/??/??) +* archive: + - close archive when stream is closed * input: - file: don't fall back to parent directory - archive: fixed memory leak in error handler diff --git a/src/archive/bz2_plugin.c b/src/archive/bz2_plugin.c index 78f13400a..0ef042e90 100644 --- a/src/archive/bz2_plugin.c +++ b/src/archive/bz2_plugin.c @@ -173,6 +173,8 @@ bz2_is_close(struct input_stream *is) bz2_context *context = (bz2_context *) is->data; bz2_destroy(context); is->data = NULL; + + bz2_close((struct archive_file *)context); } static int diff --git a/src/archive/iso_plugin.c b/src/archive/iso_plugin.c index d295f148f..7d2c075b1 100644 --- a/src/archive/iso_plugin.c +++ b/src/archive/iso_plugin.c @@ -165,6 +165,8 @@ iso_is_close(struct input_stream *is) { iso_context *context = (iso_context *) is->data; g_free(context->statbuf); + + iso_close((struct archive_file *)context); } diff --git a/src/archive/zip_plugin.c b/src/archive/zip_plugin.c index dbd2534fa..2f08b3812 100644 --- a/src/archive/zip_plugin.c +++ b/src/archive/zip_plugin.c @@ -133,6 +133,8 @@ zip_is_close(struct input_stream *is) { zip_context *context = (zip_context *) is->data; zzip_file_close (context->file); + + zip_close((struct archive_file *)context); } static size_t diff --git a/src/archive_api.h b/src/archive_api.h index dbd050bfa..2efcc1e6a 100644 --- a/src/archive_api.h +++ b/src/archive_api.h @@ -73,6 +73,9 @@ struct archive_plugin { /** * Opens an input_stream of a file within the archive. * + * If this function succeeds, then the #input_stream "owns" + * the archive file and will automatically close it. + * * @param path the path within the archive */ bool (*open_stream)(struct archive_file *, struct input_stream *is, From 9179f108a540dcd27dafeb015778cc4dd873dfe5 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Tue, 15 Dec 2009 19:56:38 +0100 Subject: [PATCH 7/7] iso, zip: fixed memory leak in destructor Free the "context" pointer in the method archive_plugin.close(). --- NEWS | 1 + src/archive/iso_plugin.c | 3 ++- src/archive/zip_plugin.c | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 7ead1f16e..cd39e9066 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,7 @@ ver 0.15.7 (2009/??/??) * archive: - close archive when stream is closed + - iso, zip: fixed memory leak in destructor * input: - file: don't fall back to parent directory - archive: fixed memory leak in error handler diff --git a/src/archive/iso_plugin.c b/src/archive/iso_plugin.c index 7d2c075b1..9063af0fc 100644 --- a/src/archive/iso_plugin.c +++ b/src/archive/iso_plugin.c @@ -132,7 +132,8 @@ iso_close(struct archive_file *file) } //close archive iso9660_close(context->iso); - context->iso = NULL; + + g_free(context); } /* single archive handling */ diff --git a/src/archive/zip_plugin.c b/src/archive/zip_plugin.c index 2f08b3812..243d46418 100644 --- a/src/archive/zip_plugin.c +++ b/src/archive/zip_plugin.c @@ -99,7 +99,8 @@ zip_close(struct archive_file *file) } //close archive zzip_dir_close (context->dir); - context->dir = NULL; + + g_free(context); } /* single archive handling */