[systemd-devel] systemd coverity

2012-08-23 Thread Lukáš Nykrýn 
Hello,
Coverity found some new small issues between releases 185-188. See attached 
patches.

Regards
Lukas

From b258ab8b5fec97e924ba5d3784be9b72d7966118 Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 20 Aug 2012 14:33:21 +0200
Subject: [PATCH 1/4] load-fragment: initialize bool invert before use

---
 src/core/load-fragment.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index 1068130..47a7f4f 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -2028,7 +2028,7 @@ int config_parse_syscall_filter(
 
 ExecContext *c = data;
 Unit *u = userdata;
-bool invert;
+bool invert = false;
 char *w;
 size_t l;
 char *state;
-- 
1.7.6.5

From 72b050672944143d846bc2059e2dec5ea7ad04fb Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 20 Aug 2012 14:39:08 +0200
Subject: [PATCH 2/4] login: check return of parse_pid and parse_uid

---
 src/login/logind-inhibit.c |   10 --
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/login/logind-inhibit.c b/src/login/logind-inhibit.c
index 96b7c6c..1803f8a 100644
--- a/src/login/logind-inhibit.c
+++ b/src/login/logind-inhibit.c
@@ -220,10 +220,16 @@ int inhibitor_load(Inhibitor *i) {
 i-mode = mm;
 
 if (uid)
-parse_uid(uid, i-uid);
+r = parse_uid(uid, i-uid);
+
+if (r  0)
+goto finish;
 
 if (pid)
-parse_pid(pid, i-pid);
+r = parse_pid(pid, i-pid);
+
+if (r  0)
+goto finish;
 
 if (who) {
 cc = cunescape(who);
-- 
1.7.6.5

From 36642c2aef0c441da508c1bb7ff68d69441f023f Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 20 Aug 2012 14:52:07 +0200
Subject: [PATCH 3/4] core: free word later in parse_proc_cmdline

---
 src/core/main.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/core/main.c b/src/core/main.c
index cdd77c1..16e8b35 100644
--- a/src/core/main.c
+++ b/src/core/main.c
@@ -727,12 +727,13 @@ static int parse_proc_cmdline(void) {
 }
 
 r = parse_proc_cmdline_word(word);
-free(word);
-
 if (r  0) {
 log_error(Failed on cmdline argument %s: %s, word, strerror(-r));
+free(word);
 goto finish;
 }
+
+free(word);
 }
 
 r = 0;
-- 
1.7.6.5

From e455c40879578901943ce16f000b671449a9fc5a Mon Sep 17 00:00:00 2001
From: Lukas Nykryn lnyk...@redhat.com
Date: Mon, 20 Aug 2012 15:15:40 +0200
Subject: [PATCH 4/4] readahead-analyze: don't call fclose on null

---
 src/readahead/readahead-analyze.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/readahead/readahead-analyze.c b/src/readahead/readahead-analyze.c
index 11b2b2d..9a929c0 100644
--- a/src/readahead/readahead-analyze.c
+++ b/src/readahead/readahead-analyze.c
@@ -144,6 +144,7 @@ int main_analyze(const char *pack_path) {
 return EXIT_SUCCESS;
 
 fail:
-fclose(pack);
+if(pack)
+fclose(pack);
 return EXIT_FAILURE;
 }
-- 
1.7.6.5

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Zbigniew Jędrzejewski-Szmek 
On 08/23/2012 09:25 AM, Lukáš Nykrýn wrote:
 Hello,
 Coverity found some new small issues between releases 185-188. See attached 
 patches.
Applied all, with small change in 'login: check return of parse_pid and
parse_uid'.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Zbigniew Jędrzejewski-Szmek 
On 08/23/2012 12:36 PM, Lukáš Nykrýn wrote:
 Hello,
 Two more patches from diff scan 188-189.
Applied both.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Lennart Poettering 
On Thu, 23.08.12 13:57, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

 On 08/23/2012 12:36 PM, Lukáš Nykrýn wrote:
  Hello,
  Two more patches from diff scan 188-189.
 Applied both.

I think you forgot to push?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Lennart Poettering 
On Thu, 23.08.12 09:25, Lukáš Nykrýn (lnyk...@redhat.com) wrote:

 
 diff --git a/src/readahead/readahead-analyze.c 
 b/src/readahead/readahead-analyze.c
 index 11b2b2d..9a929c0 100644
 --- a/src/readahead/readahead-analyze.c
 +++ b/src/readahead/readahead-analyze.c
 @@ -144,6 +144,7 @@ int main_analyze(const char *pack_path) {
  return EXIT_SUCCESS;
  
  fail:
 -fclose(pack);
 +if(pack)
 +fclose(pack);
  return EXIT_FAILURE;
  }

I am wondering whether we should begin making use of gcc's cleanup
variable attribute to avoid problems with cleaning up dynamicly
allocated objects. That way we wouldn't have to manually clean up things
like this anymore, so we'd have less chance to forget it, to check for
NULL before and other things.

I think we should be conservative with the cleanup stuff and only use it
for very low level objects, not all objects though. i.e. stuff that only
needs free() or another glibc call to destruct sounds good, but I
wouldn't use it for stuff that needs a more complex destruction logic.

maybe we should add macros like:

#define _cleanup_free_ __attribute__((cleanup(freep)))
#define _cleanup_fclose_ __attribute__((cleanup(fclosep)))

and a few more. (Assuming that freep()/fclosep() are defined as needed).

Then, whenever we allocate something on the heap and need it only within
one function we'd write this:

_cleanup_free_ char *s = NULL;

instead of:

char *s = NULL;
...
free(s);

What do you think?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Zbigniew Jędrzejewski-Szmek 
On 08/23/2012 02:36 PM, Lennart Poettering wrote:
 maybe we should add macros like:
 
 #define _cleanup_free_ __attribute__((cleanup(freep)))
 #define _cleanup_fclose_ __attribute__((cleanup(fclosep)))
 
 What do you think?
I personally think that this would be a welcome change like the
#pragma once cleanup.

__attribute__(cleanup) goes all the way back to gcc 3.3, so there's
little reason not to use it.


On a related topic:
maybe gotos should be used more often for structured cleanup.
This might make the code slightly shorted, and also help avoid
mistakes.

diff --git src/journal/sd-journal.c src/journal/sd-journal.c
index 0f7c02c..5e73a94 100644
--- src/journal/sd-journal.c
+++ src/journal/sd-journal.c
@@ -1418,37 +1418,33 @@ static sd_journal *journal_new(int flags, const
char *path) {

 if (path) {
 j-path = strdup(path);
-if (!j-path) {
-free(j);
-return NULL;
-}
+if (!j-path)
+goto free_1;
 }

 j-files = hashmap_new(string_hash_func, string_compare_func);
-if (!j-files) {
-free(j-path);
-free(j);
-return NULL;
-}
+if (!j-files)
+goto free_2;

 j-directories_by_path = hashmap_new(string_hash_func,
string_compare_func);
-if (!j-directories_by_path) {
-hashmap_free(j-files);
-free(j-path);
-free(j);
-return NULL;
-}
+if (!j-directories_by_path)
+goto free_3;

 j-mmap = mmap_cache_new();
-if (!j-mmap) {
-hashmap_free(j-files);
-hashmap_free(j-directories_by_path);
-free(j-path);
-free(j);
-return NULL;
-}
+if (!j-mmap)
+goto free_4;

 return j;
+
+free_4:
+hashmap_free(j-files);
+free_3:
+hashmap_free(j-directories_by_path);
+free_2:
+free(j-path);
+free_1:
+free(j);
+return NULL;
 }
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Daniel P. Berrange 
On Thu, Aug 23, 2012 at 03:04:23PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
 On 08/23/2012 02:36 PM, Lennart Poettering wrote:
  maybe we should add macros like:
  
  #define _cleanup_free_ __attribute__((cleanup(freep)))
  #define _cleanup_fclose_ __attribute__((cleanup(fclosep)))
  
  What do you think?
 I personally think that this would be a welcome change like the
 #pragma once cleanup.
 
 __attribute__(cleanup) goes all the way back to gcc 3.3, so there's
 little reason not to use it.
 
 
 On a related topic:
 maybe gotos should be used more often for structured cleanup.
 This might make the code slightly shorted, and also help avoid
 mistakes.
 
 diff --git src/journal/sd-journal.c src/journal/sd-journal.c
 index 0f7c02c..5e73a94 100644
 --- src/journal/sd-journal.c
 +++ src/journal/sd-journal.c
 @@ -1418,37 +1418,33 @@ static sd_journal *journal_new(int flags, const
 char *path) {
 
  if (path) {
  j-path = strdup(path);
 -if (!j-path) {
 -free(j);
 -return NULL;
 -}
 +if (!j-path)
 +goto free_1;
  }
 
  j-files = hashmap_new(string_hash_func, string_compare_func);
 -if (!j-files) {
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-files)
 +goto free_2;
 
  j-directories_by_path = hashmap_new(string_hash_func,
 string_compare_func);
 -if (!j-directories_by_path) {
 -hashmap_free(j-files);
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-directories_by_path)
 +goto free_3;
 
  j-mmap = mmap_cache_new();
 -if (!j-mmap) {
 -hashmap_free(j-files);
 -hashmap_free(j-directories_by_path);
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-mmap)
 +goto free_4;
 
  return j;
 +
 +free_4:
 +hashmap_free(j-files);
 +free_3:
 +hashmap_free(j-directories_by_path);
 +free_2:
 +free(j-path);
 +free_1:
 +free(j);

If you make sure that your pointer vars are all initialized to NULL,
and that all free functions accept NULL, then you can collapse all
those separate labels into one. This is much nicer, because then
you don't need to go about re-numbering if you need to insert another
goto in the middle of the function.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] systemd coverity

2012-08-23 Thread Zbigniew Jędrzejewski-Szmek 
On 08/23/2012 03:11 PM, Daniel P. Berrange wrote:
 On Thu, Aug 23, 2012 at 03:04:23PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
 On 08/23/2012 02:36 PM, Lennart Poettering wrote:
 maybe we should add macros like:

 #define _cleanup_free_ __attribute__((cleanup(freep)))
 #define _cleanup_fclose_ __attribute__((cleanup(fclosep)))

 What do you think?
 I personally think that this would be a welcome change like the
 #pragma once cleanup.

 __attribute__(cleanup) goes all the way back to gcc 3.3, so there's
 little reason not to use it.


 On a related topic:
 maybe gotos should be used more often for structured cleanup.
 This might make the code slightly shorted, and also help avoid
 mistakes.

 diff --git src/journal/sd-journal.c src/journal/sd-journal.c
 index 0f7c02c..5e73a94 100644
 --- src/journal/sd-journal.c
 +++ src/journal/sd-journal.c
 @@ -1418,37 +1418,33 @@ static sd_journal *journal_new(int flags, const
 char *path) {

  if (path) {
  j-path = strdup(path);
 -if (!j-path) {
 -free(j);
 -return NULL;
 -}
 +if (!j-path)
 +goto free_1;
  }

  j-files = hashmap_new(string_hash_func, string_compare_func);
 -if (!j-files) {
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-files)
 +goto free_2;

  j-directories_by_path = hashmap_new(string_hash_func,
 string_compare_func);
 -if (!j-directories_by_path) {
 -hashmap_free(j-files);
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-directories_by_path)
 +goto free_3;

  j-mmap = mmap_cache_new();
 -if (!j-mmap) {
 -hashmap_free(j-files);
 -hashmap_free(j-directories_by_path);
 -free(j-path);
 -free(j);
 -return NULL;
 -}
 +if (!j-mmap)
 +goto free_4;

  return j;
 +
 +free_4:
 +hashmap_free(j-files);
 +free_3:
 +hashmap_free(j-directories_by_path);
 +free_2:
 +free(j-path);
 +free_1:
 +free(j);
 
 If you make sure that your pointer vars are all initialized to NULL,
 and that all free functions accept NULL, then you can collapse all
 those separate labels into one. This is much nicer, because then
 you don't need to go about re-numbering if you need to insert another
 goto in the middle of the function.
It is also necessary to make sure that j isn't null in the free_4-2.
But indeed they can be collapsed.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel