On Wed, Apr 25, 2012 at 04:07:29PM +0200, Sumit Bose wrote:
> On Wed, Apr 25, 2012 at 03:28:20PM +0200, Stef Walter wrote:
> > On 04/24/2012 12:42 PM, Sumit Bose wrote:
> > > Chances are that some static code analysis tools or -D_FORTIFY_SOURCE=2
> > > might complain about an unchecked return value. Currently we mostly try
> > > to make those tools happy, even if the code becomes a bit redundant.
> > >
> > > Have you checked if -D_FORTIFY_SOURCE=2 gives an extra warning here?
> >
> > Do I just define that macro? ie:
> >
> > $ CFLAGS='-g -O0 -D_FORTIFY_SOURCE=2' ./configure ...
>
> yes, this is ok. Typically it is used as -Wp,-D_FORTIFY_SOURCE=2, but
> your usage would work as well.
> >
> > If that's the case, then no, no additional warnings were generated.
>
> ok, thank you for testing.
>
> bye,
> Sumit
>
I also tested that clang's scan-build does not emit any new warning with
the patch -- it doesn't. It *did* print a handful of warnings in the
existing code, so I'm looking into them to sort out the real bugs from
the false positives.
I'm also attaching the patch Stef attached to the ticket. I don't have
any more comments, so Ack from me.
>From c3c07fc8218ce59051ec2101a1aed8416df3d18a Mon Sep 17 00:00:00 2001
From: Stef Walter <st...@gnome.org>
Date: Tue, 24 Apr 2012 11:32:04 +0200
Subject: [PATCH] execv, excvp and exec_child never return EOK
* So don't need to handle that case
---
src/providers/ipa/ipa_dyndns.c | 8 +++-----
src/providers/ldap/sdap_child_helpers.c | 8 +++-----
src/util/child_common.c | 12 ++++--------
src/util/child_common.h | 1 +
4 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
index 71fc27b..4224919 100644
--- a/src/providers/ipa/ipa_dyndns.c
+++ b/src/providers/ipa/ipa_dyndns.c
@@ -1015,11 +1015,9 @@ fork_nsupdate_send(struct ipa_nsupdate_ctx *ctx)
}
errno = 0;
- ret = execv(NSUPDATE_PATH, args);
- if(ret == -1) {
- err = errno;
- DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
- }
+ execv(NSUPDATE_PATH, args);
+ err = errno;
+ DEBUG(SSSDBG_CRIT_FAILURE, ("execv failed [%d][%s].\n", err,
strerror(err)));
return NULL;
}
diff --git a/src/providers/ldap/sdap_child_helpers.c
b/src/providers/ldap/sdap_child_helpers.c
index 704c89e..eeb5e5f 100644
--- a/src/providers/ldap/sdap_child_helpers.c
+++ b/src/providers/ldap/sdap_child_helpers.c
@@ -107,11 +107,9 @@ static errno_t sdap_fork_child(struct tevent_context *ev,
err = exec_child(child,
pipefd_to_child, pipefd_from_child,
LDAP_CHILD, ldap_child_debug_fd);
- if (err != EOK) {
- DEBUG(1, ("Could not exec LDAP child: [%d][%s].\n",
- err, strerror(err)));
- return err;
- }
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Could not exec LDAP child: [%d][%s].\n",
+ err, strerror(err)));
+ return err;
} else if (pid > 0) { /* parent */
child->pid = pid;
child->read_from_child_fd = pipefd_from_child[0];
diff --git a/src/util/child_common.c b/src/util/child_common.c
index 05f00b0..5f0b265 100644
--- a/src/util/child_common.c
+++ b/src/util/child_common.c
@@ -695,14 +695,10 @@ errno_t exec_child(TALLOC_CTX *mem_ctx,
return ret;
}
- ret = execv(binary, argv);
- if (ret == -1) {
- err = errno;
- DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
- return err;
- }
-
- return EOK;
+ execv(binary, argv);
+ err = errno;
+ DEBUG(SSSDBG_OP_FAILURE, ("execv failed [%d][%s].\n", err, strerror(err)));
+ return err;
}
void child_cleanup(int readfd, int writefd)
diff --git a/src/util/child_common.h b/src/util/child_common.h
index 1e9f1b6..237969f 100644
--- a/src/util/child_common.h
+++ b/src/util/child_common.h
@@ -100,6 +100,7 @@ void child_sig_handler(struct tevent_context *ev,
struct tevent_signal *sige, int signum,
int count, void *__siginfo, void *pvt);
+/* Never returns EOK, ether returns an error, or doesn't return on success */
errno_t exec_child(TALLOC_CTX *mem_ctx,
int *pipefd_to_child, int *pipefd_from_child,
const char *binary, int debug_fd);
--
1.7.10
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
