[pacman-dev] [PATCH 0/4] Smatch warning fixes

newer
[pacman-dev] [PATCH 1/4] Recommend...

older
[pacman-dev] [PATCH 1/6] pacman.c:...

Silvan Jegen

28 Jan 2014 28 Jan '14

4:50 p.m.

Hi I ran smatch[1] on the pacman source code and it reported a few minor issues. Three out of four of the following patches are checks for NULL when freeing memory which are strictly not necessary since calling free with a NULL argument is a no-op. The last patch moves a NULL check before a pointer dereference which should be the correct place to put it. Cheers, Silvan [1] http://smatch.sourceforge.net/ Silvan Jegen (4): Remove unneeded NULL check Another unneeded NULL check removed Another NULL check removed Move NULL check before dereference src/pacman/pacman.c | 16 +++++++--------- src/pacman/util.c | 10 ++++------ src/util/pacsort.c | 5 +---- 3 files changed, 12 insertions(+), 19 deletions(-) -- 1.8.5.3

Show replies by date

Silvan Jegen

28 Jan 28 Jan

4:50 p.m.

New subject: [pacman-dev] [PATCH 1/4] Remove unneeded NULL check

Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 6bf94e9..7329f0f 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -421,9 +421,7 @@ static int parsearg_global(int opt) break; case OP_CONFIG: check_optarg(); - if(config->configfile) { - free(config->configfile); - } + free(config->configfile); config->configfile = strndup(optarg, PATH_MAX); break; case OP_DEBUG: -- 1.8.5.3

Silvan Jegen

4:50 p.m.

New subject: [pacman-dev] [PATCH 2/4] Another unneeded NULL check removed

Signed-off-by: Silvan Jegen --- src/pacman/util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/pacman/util.c b/src/pacman/util.c index 58b0cec..4aaa98f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -206,12 +206,10 @@ int rmrf(const char *path) return 1; } for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { - if(dp->d_name) { - if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { - char name[PATH_MAX]; - snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); - errflag += rmrf(name); - } + if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { + char name[PATH_MAX]; + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); + errflag += rmrf(name); } } closedir(dirp); -- 1.8.5.3

Dan McGee

4:58 p.m.

New subject: [pacman-dev] [PATCH 2/4] Another unneeded NULL check removed

On Tue, Jan 28, 2014 at 10:50 AM, Silvan Jegen wrote:

...

Signed-off-by: Silvan Jegen --- src/pacman/util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/pacman/util.c b/src/pacman/util.c index 58b0cec..4aaa98f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -206,12 +206,10 @@ int rmrf(const char *path) return 1; } for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { - if(dp->d_name) { - if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { - char name[PATH_MAX]; - snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); - errflag += rmrf(name); - } + if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { + char name[PATH_MAX]; + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); + errflag += rmrf(name);

I'm failing to see how this one is valid. Please explain? I'm not sure the original code is right (we should likely be checking that d_name[0] is != '\0;), but removing the check completely seems heavy-handed.

...

} } closedir(dirp); -- 1.8.5.3

Silvan Jegen

5:33 p.m.

New subject: [pacman-dev] [PATCH 2/4] Another unneeded NULL check removed

On Tue, Jan 28, 2014 at 10:58:30AM -0600, Dan McGee wrote:

...

On Tue, Jan 28, 2014 at 10:50 AM, Silvan Jegen wrote:

...
Signed-off-by: Silvan Jegen --- src/pacman/util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/pacman/util.c b/src/pacman/util.c index 58b0cec..4aaa98f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -206,12 +206,10 @@ int rmrf(const char *path) return 1; } for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { - if(dp->d_name) { - if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { - char name[PATH_MAX]; - snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); - errflag += rmrf(name); - } + if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { + char name[PATH_MAX]; + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); + errflag += rmrf(name);

I'm failing to see how this one is valid. Please explain? I'm not sure the original code is right (we should likely be checking that d_name[0] is != '\0;), but removing the check completely seems heavy-handed.

My reasoning was very simple. I assume that each directory entry MUST have a filename (that only contains a terminating NULL character) so checking whether the d_name field of the dirent struct is NULL should not be necessary. If I missed a case where the filename can actually be NULL then just ignore this one.

Dan McGee

11:26 p.m.

New subject: [pacman-dev] [PATCH 2/4] Another unneeded NULL check removed

...

...
On Tue, Jan 28, 2014 at 10:50 AM, Silvan Jegen wrote:

...
Signed-off-by: Silvan Jegen --- src/pacman/util.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/pacman/util.c b/src/pacman/util.c index 58b0cec..4aaa98f 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -206,12 +206,10 @@ int rmrf(const char *path) return 1; } for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { - if(dp->d_name) { - if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { - char name[PATH_MAX]; - snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); - errflag += rmrf(name); - } + if(strcmp(dp->d_name, "..") != 0 && strcmp(dp->d_name, ".") != 0) { + char name[PATH_MAX]; + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); + errflag += rmrf(name);

I'm failing to see how this one is valid. Please explain? I'm not sure

On Tue, Jan 28, 2014 at 10:58:30AM -0600, Dan McGee wrote: the

...
original code is right (we should likely be checking that d_name[0] is != '\0;), but removing the check completely seems heavy-handed.

My reasoning was very simple. I assume that each directory entry MUST have a filename (that only contains a terminating NULL character) so checking whether the d_name field of the dirent struct is NULL should not be necessary.

If I missed a case where the filename can actually be NULL then just ignore this one.

Looking at it more, I think you're right here, especially after I trace

On Tue, Jan 28, 2014 at 11:33 AM, Silvan Jegen wrote: the history of this code. I'm OK with this change. We no longer do it in the backend either (see commit 1b50223f for some history). -Dan

Silvan Jegen

4:50 p.m.

New subject: [pacman-dev] [PATCH 3/4] Another NULL check removed

Signed-off-by: Silvan Jegen --- src/util/pacsort.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 443e822..df3e809 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -97,10 +97,7 @@ static void buffer_free(struct buffer_t *buf) return; } - if(buf->mem) { - free(buf->mem); - } - + free(buf->mem); free(buf); } -- 1.8.5.3

Silvan Jegen

4:50 p.m.

New subject: [pacman-dev] [PATCH 4/4] Move NULL check before dereference

Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..1d821ef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -262,13 +262,13 @@ static void setuseragent(void) static void cleanup(int ret) { /* free alpm library resources */ - if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + + /* free memory */ + FREELIST(pm_targets); config_free(config); config = NULL; } -- 1.8.5.3

Andrew Gregory

5:32 p.m.

New subject: [pacman-dev] [PATCH 4/4] Move NULL check before dereference

On 01/28/14 at 05:50pm, Silvan Jegen wrote:

...

Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..1d821ef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -262,13 +262,13 @@ static void setuseragent(void) static void cleanup(int ret) { /* free alpm library resources */

This comment applies specifically to the alpm_release call and either needs to be moved with it or deleted.

...

- if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + + /* free memory */ + FREELIST(pm_targets);

pm_targets is independent of config and needs to be freed whether config is NULL or not.

...

config_free(config); config = NULL; } -- 1.8.5.3

Silvan Jegen

5:58 p.m.

New subject: [pacman-dev] [PATCH 4/4] Move NULL check before dereference

On Tue, Jan 28, 2014 at 12:32:13PM -0500, Andrew Gregory wrote:

...

On 01/28/14 at 05:50pm, Silvan Jegen wrote:

...
Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..1d821ef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -262,13 +262,13 @@ static void setuseragent(void) static void cleanup(int ret) { /* free alpm library resources */

This comment applies specifically to the alpm_release call and either needs to be moved with it or deleted.

My bad.

...

...
- if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + + /* free memory */ + FREELIST(pm_targets);

pm_targets is independent of config and needs to be freed whether config is NULL or not.

Ditto. A corrected version of the patch can be found below. Hope I got it right this time. -->8-- (for use with git am --scissors) Subject: [PATCH v2] Move NULL check before dereference Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..3ecced5 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -261,18 +261,18 @@ static void setuseragent(void) */ static void cleanup(int ret) { - /* free alpm library resources */ - if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + /* free alpm library resources */ + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + config_free(config); config = NULL; } + /* free memory */ + FREELIST(pm_targets); exit(ret); } -- 1.8.5.3

Allan McRae

30 Jan 30 Jan

3:32 a.m.

New subject: [pacman-dev] [PATCH 4/4] Move NULL check before dereference

On 29/01/14 03:58, Silvan Jegen wrote:

...

On Tue, Jan 28, 2014 at 12:32:13PM -0500, Andrew Gregory wrote:

...
On 01/28/14 at 05:50pm, Silvan Jegen wrote:

...
Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..1d821ef 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -262,13 +262,13 @@ static void setuseragent(void) static void cleanup(int ret) { /* free alpm library resources */

This comment applies specifically to the alpm_release call and either needs to be moved with it or deleted.

My bad.

...
...
- if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + + /* free memory */ + FREELIST(pm_targets);

pm_targets is independent of config and needs to be freed whether config is NULL or not.

Ditto.

A corrected version of the patch can be found below. Hope I got it right this time.

-->8-- (for use with git am --scissors) Subject: [PATCH v2] Move NULL check before dereference

Signed-off-by: Silvan Jegen --- src/pacman/pacman.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7329f0f..3ecced5 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -261,18 +261,18 @@ static void setuseragent(void) */ static void cleanup(int ret) { - /* free alpm library resources */ - if(config->handle && alpm_release(config->handle) == -1) { - pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); - } - - /* free memory */ - FREELIST(pm_targets); if(config) { + /* free alpm library resources */ + if(config->handle && alpm_release(config->handle) == -1) { + pm_printf(ALPM_LOG_ERROR, "error releasing alpm library\n"); + } + config_free(config); config = NULL; }

+ /* free memory */ + FREELIST(pm_targets); exit(ret); }

This version looks fine. Allan

3732

Age (days ago)

3734

Last active (days ago)

List overview

Download

10 comments

4 participants

participants (4)

Allan McRae
Andrew Gregory
Dan McGee
Silvan Jegen

[pacman-dev] [PATCH 0/4] Smatch warning fixes

tags

participants (4)