[PATCH] SELINUX: Check the return value of setuid and setgid

[Jakub Hrozek]

Hi,

As the automated test tools of our downstream discovered, selinux_child
now compiles with a warning if -Wunused-result is set:

sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring return value of 'setgid', declared with attribute warn_unused_result [-Wunused-result]
sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring return value of 'setuid', declared with attribute warn_unused_result [-Wunused-result]

Sorry I didn't catch this earlier, but I don't think we can do much about
the error anyway, just warn.

[Pavel Reichl]

On 01/27/2015 08:35 PM, Jakub Hrozek wrote:
>    */
>   if (getuid() != 0) {
>    setuid(0);
>    errno = 0;

I don't think we need to null errno in this case
>    ret = setuid(0);
>    if (ret == -1) {
>        ret = errno;
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "setuid failed: %d, selinux_child might not work!\n", ret);
>    }
> }
>
> if (getgid() != 0) {
>    setgid(0);
>    errno = 0;

same here
>    ret = setgid(0);
>    if (ret == -1) {
>        ret = errno;
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "setgid failed: %d, selinux_child might not work!\n", ret);
>    }
> }

Patch looks good to me. I have just a nitpick about nulling errno.

After applying the patch clang-analyser no longer reports the warnings.

[Pavel Reichl]

On 01/28/2015 09:29 AM, Pavel Reichl wrote:
> On 01/27/2015 08:35 PM, Jakub Hrozek wrote:
> >    */
> >   if (getuid() != 0) {
> >    setuid(0);
> >    errno = 0;
>
> I don't think we need to null errno in this case
> >    ret = setuid(0);
> >    if (ret == -1) {
> >        ret = errno;
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "setuid failed: %d, selinux_child might not 
> >
> > work!\n", ret);
> >    }
> > }
> >   if (getgid() != 0) {
> >    setgid(0);
> >    errno = 0;
>
> same here
> >    ret = setgid(0);
> >    if (ret == -1) {
> >        ret = errno;
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "setgid failed: %d, selinux_child might not 
> >
> > work!\n", ret);
> >    }
> > }
>
> Patch looks good to me. I have just a nitpick about nulling errno.
>
> After applying the patch clang-analyser no longer reports the warnings.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Jakub feel free to push the patch without fixing the nitpicks.

[Jakub Hrozek]

On Thu, Feb 12, 2015 at 04:29:31PM +0100, Pavel Reichl wrote:
> On 01/28/2015 09:29 AM, Pavel Reichl wrote:
> > On 01/27/2015 08:35 PM, Jakub Hrozek wrote:
> > >   */
> > >  if (getuid() != 0) {
> > >    setuid(0);
> > >    errno = 0;
> >
> > I don't think we need to null errno in this case
> > >    ret = setuid(0);
> > >    if (ret == -1) {
> > >        ret = errno;
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "setuid failed: %d, selinux_child might not work!\n",
> > >
> > > ret);
> > >    }
> > > }
> > >      if (getgid() != 0) {
> > >    setgid(0);
> > >    errno = 0;
> >
> > same here
> > >    ret = setgid(0);
> > >    if (ret == -1) {
> > >        ret = errno;
> > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > >              "setgid failed: %d, selinux_child might not work!\n",
> > >
> > > ret);
> > >    }
> > > }
> >
> > Patch looks good to me. I have just a nitpick about nulling errno.
> >
> > After applying the patch clang-analyser no longer reports the warnings.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Jakub feel free to push the patch without fixing the nitpicks.

No need to, I simply forgot about the patch :-)

sorry, new version attached.

[Pavel Reichl]

On 02/13/2015 05:27 PM, Jakub Hrozek wrote:
> On Thu, Feb 12, 2015 at 04:29:31PM +0100, Pavel Reichl wrote:
> > On 01/28/2015 09:29 AM, Pavel Reichl wrote:
> > > On 01/27/2015 08:35 PM, Jakub Hrozek wrote:
> > > >    */
> > > >   if (getuid() != 0) {
> > > >    setuid(0);
> > > >    errno = 0;
> > >
> > > I don't think we need to null errno in this case
> > > >    ret = setuid(0);
> > > >    if (ret == -1) {
> > > >        ret = errno;
> > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > >              "setuid failed: %d, selinux_child might not work!\n",
> > > >
> > > > ret);
> > > >    }
> > > > }
> > > >   if (getgid() != 0) {
> > > >    setgid(0);
> > > >    errno = 0;
> > >
> > > same here
> > > >    ret = setgid(0);
> > > >    if (ret == -1) {
> > > >        ret = errno;
> > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > >              "setgid failed: %d, selinux_child might not work!\n",
> > > >
> > > > ret);
> > > >    }
> > > > }
> > >
> > > Patch looks good to me. I have just a nitpick about nulling errno.
> > >
> > > After applying the patch clang-analyser no longer reports the warnings.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Jakub feel free to push the patch without fixing the nitpicks.
>
> No need to, I simply forgot about the patch :-)
>
> sorry, new version attached.

Thanks. ACK.

clang-analyzer  didn't report any other problems in selinux_child.c

http://sssd-ci.duckdns.org/logs/job/7/34/summary.html
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Feb 13, 2015 at 06:16:48PM +0100, Pavel Reichl wrote:
> On 02/13/2015 05:27 PM, Jakub Hrozek wrote:
> > On Thu, Feb 12, 2015 at 04:29:31PM +0100, Pavel Reichl wrote:
> > > On 01/28/2015 09:29 AM, Pavel Reichl wrote:
> > > > On 01/27/2015 08:35 PM, Jakub Hrozek wrote:
> > > > >   */
> > > > >  if (getuid() != 0) {
> > > > >    setuid(0);
> > > > >    errno = 0;
> > > >
> > > > I don't think we need to null errno in this case
> > > > >    ret = setuid(0);
> > > > >    if (ret == -1) {
> > > > >        ret = errno;
> > > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > >              "setuid failed: %d, selinux_child might not work!\n",
> > > > >
> > > > > ret);
> > > > >    }
> > > > > }
> > > > >      if (getgid() != 0) {
> > > > >    setgid(0);
> > > > >    errno = 0;
> > > >
> > > > same here
> > > > >    ret = setgid(0);
> > > > >    if (ret == -1) {
> > > > >        ret = errno;
> > > > >        DEBUG(SSSDBG_CRIT_FAILURE,
> > > > >              "setgid failed: %d, selinux_child might not work!\n",
> > > > >
> > > > > ret);
> > > > >    }
> > > > > }
> > > >
> > > > Patch looks good to me. I have just a nitpick about nulling errno.
> > > >
> > > > After applying the patch clang-analyser no longer reports the warnings.
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > Jakub feel free to push the patch without fixing the nitpicks.
> >
> > No need to, I simply forgot about the patch :-)
> >
> > sorry, new version attached.
>
> Thanks. ACK.
>
> clang-analyzer  didn't report any other problems in selinux_child.c
>
> http://sssd-ci.duckdns.org/logs/job/7/34/summary.html

* master: b0f46a3019e0ff4f375ef07682ceb9418751707f
* sssd-1-12: dc13b1aff629b0271eb6b75a9f3bdb43c9767093

[Roland Mainz]

----- Original Message -----
> From: "Jakub Hrozek" 
> To: [e-mail]
> Sent: Tuesday, January 27, 2015 8:35:20 PM
> Subject: [SSSD] [PATCH] SELINUX: Check the return value of setuid and setgid
>
> Hi,
>
> As the automated test tools of our downstream discovered, selinux_child
> now compiles with a warning if -Wunused-result is set:
>
> sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring
> return value of 'setgid', declared with attribute warn_unused_result
> [-Wunused-result]
> sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring
> return value of 'setuid', declared with attribute warn_unused_result
> [-Wunused-result]
>
> Sorry I didn't catch this earlier, but I don't think we can do much about
> the error anyway, just warn.

Does the warning go away if you cast the result to |(void)|, e.g. |(void)setuid(666)| ? Classical UNIX programming coding style (basically enforced by lint(1) defaults) is that each unused function return value must be casted to |(void)|.

----

Bye,
Roland

[Jakub Hrozek]

On Thu, Feb 12, 2015 at 04:56:54AM -0500, Roland Mainz wrote:
> ----- Original Message -----
> > From: "Jakub Hrozek" 
> > To: [e-mail]
> > Sent: Tuesday, January 27, 2015 8:35:20 PM
> > Subject: [SSSD] [PATCH] SELINUX: Check the return value of setuid and setgid
> >
> > Hi,
> >
> > As the automated test tools of our downstream discovered, selinux_child
> > now compiles with a warning if -Wunused-result is set:
> >
> > sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring
> > return value of 'setgid', declared with attribute warn_unused_result
> > [-Wunused-result]
> > sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring
> > return value of 'setuid', declared with attribute warn_unused_result
> > [-Wunused-result]
> >
> > Sorry I didn't catch this earlier, but I don't think we can do much about
> > the error anyway, just warn.
>
> Does the warning go away if you cast the result to |(void)|, e.g. |(void)setuid(666)| ? Classical UNIX programming coding style (basically enforced by lint(1) defaults) is that each unused function return value must be casted to |(void)|.

That would work, too, but I think logging the failure is more user
friendly.

[Roland Mainz]

----- Original Message -----
> From: "Jakub Hrozek" 
> To: [e-mail]
> Sent: Friday, February 13, 2015 5:31:40 PM
> Subject: Re: [SSSD] [PATCH] SELINUX: Check the return value of setuid and setgid
>
> On Thu, Feb 12, 2015 at 04:56:54AM -0500, Roland Mainz wrote:
> > ----- Original Message -----
> > > From: "Jakub Hrozek" 
> > > To: [e-mail]
> > > Sent: Tuesday, January 27, 2015 8:35:20 PM
> > > Subject: [SSSD] [PATCH] SELINUX: Check the return value of setuid and
> > > setgid
> > >
> > > Hi,
> > >
> > > As the automated test tools of our downstream discovered, selinux_child
> > > now compiles with a warning if -Wunused-result is set:
> > >
> > > sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring
> > > return value of 'setgid', declared with attribute warn_unused_result
> > > [-Wunused-result]
> > > sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring
> > > return value of 'setuid', declared with attribute warn_unused_result
> > > [-Wunused-result]
> > >
> > > Sorry I didn't catch this earlier, but I don't think we can do much about
> > > the error anyway, just warn.
> >
> > Does the warning go away if you cast the result to |(void)|, e.g.
> > |(void)setuid(666)| ? Classical UNIX programming coding style (basically
> > enforced by lint(1) defaults) is that each unused function return value
> > must be casted to |(void)|.
>
> That would work, too, but I think logging the failure is more user
> friendly.

OK...

... slightly offtopic: Do you know any gcc warning option which can emulate lint(1)'s warning for unused/unconsumed return values ?

----

Bye,
Roland

[Jakub Hrozek]

> On 17 Feb 2015, at 03:15, Roland Mainz  wrote:
>
> ----- Original Message -----
> > From: "Jakub Hrozek" 
> > To: [e-mail]
> > Sent: Friday, February 13, 2015 5:31:40 PM
> > Subject: Re: [SSSD] [PATCH] SELINUX: Check the return value of setuid and setgid
> >
> > On Thu, Feb 12, 2015 at 04:56:54AM -0500, Roland Mainz wrote:
> > > ----- Original Message -----
> > > > From: "Jakub Hrozek" 
> > > > To: [e-mail]
> > > > Sent: Tuesday, January 27, 2015 8:35:20 PM
> > > > Subject: [SSSD] [PATCH] SELINUX: Check the return value of setuid and
> > > > setgid
> > > >
> > > > Hi,
> > > >
> > > > As the automated test tools of our downstream discovered, selinux_child
> > > > now compiles with a warning if -Wunused-result is set:
> > > >
> > > > sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring
> > > > return value of 'setgid', declared with attribute warn_unused_result
> > > > [-Wunused-result]
> > > > sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring
> > > > return value of 'setuid', declared with attribute warn_unused_result
> > > > [-Wunused-result]
> > > >
> > > > Sorry I didn't catch this earlier, but I don't think we can do much about
> > > > the error anyway, just warn.
> > >
> > > Does the warning go away if you cast the result to |(void)|, e.g.
> > > |(void)setuid(666)| ? Classical UNIX programming coding style (basically
> > > enforced by lint(1) defaults) is that each unused function return value
> > > must be casted to |(void)|.
> >
> > That would work, too, but I think logging the failure is more user
> > friendly.
>
> OK...
>
> ... slightly offtopic: Do you know any gcc warning option which can emulate lint(1)'s warning for unused/unconsumed return values ?

Normally Coverity is able to detect patterns in unused return values and warn if the return value is consumed N times but N+1th call doesn't consume it...
> Bye,
> Roland
>
> -- 
>  __ .  . __
> (o.\ / /.o) [e-mail]
>  __//__/  IPA/Kerberos5 team
>  /O /==\ O\  
> (;O/ / \O;)
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (16/02/15 21:15), Roland Mainz wrote:
> ----- Original Message -----
> > From: "Jakub Hrozek" 
> > To: [e-mail]
> > Sent: Friday, February 13, 2015 5:31:40 PM
> > Subject: Re: [SSSD] [PATCH] SELINUX: Check the return value of setuid and setgid
> >
> > On Thu, Feb 12, 2015 at 04:56:54AM -0500, Roland Mainz wrote:
> > > ----- Original Message -----
> > > > From: "Jakub Hrozek" 
> > > > To: [e-mail]
> > > > Sent: Tuesday, January 27, 2015 8:35:20 PM
> > > > Subject: [SSSD] [PATCH] SELINUX: Check the return value of setuid and
> > > > setgid
> > > >
> > > > Hi,
> > > >
> > > > As the automated test tools of our downstream discovered, selinux_child
> > > > now compiles with a warning if -Wunused-result is set:
> > > >
> > > > sssd-1.12.2/src/providers/ipa/selinux_child.c:227:15: warning: ignoring
> > > > return value of 'setgid', declared with attribute warn_unused_result
> > > > [-Wunused-result]
> > > > sssd-1.12.2/src/providers/ipa/selinux_child.c:223:15: warning: ignoring
> > > > return value of 'setuid', declared with attribute warn_unused_result
> > > > [-Wunused-result]
> > > >
> > > > Sorry I didn't catch this earlier, but I don't think we can do much about
> > > > the error anyway, just warn.
> > >
> > > Does the warning go away if you cast the result to |(void)|, e.g.
> > > |(void)setuid(666)| ? Classical UNIX programming coding style (basically
> > > enforced by lint(1) defaults) is that each unused function return value
> > > must be casted to |(void)|.
> >
> > That would work, too, but I think logging the failure is more user
> > friendly.
>
> OK...
>
> ... slightly offtopic: Do you know any gcc warning option which can emulate lint(1)'s warning for unused/unconsumed return values ?

The answer is: yes it has [-Wunused-result]

src/providers/ipa/selinux_child.c: In function 'main':
src/providers/ipa/selinux_child.c:223:15: warning: ignoring return value of 'setuid', declared with attribute warn_unused_result [-Wunused-result]
         setuid(0);
               ^
src/providers/ipa/selinux_child.c:227:15: warning: ignoring return value of 'setgid', declared with attribute warn_unused_result [-Wunused-result]
         setgid(0);
               ^

LS