[systemd-devel] [PATCH 2/2] tty-ask-password-agent: fix CID 996261

[Thomas H.P. Andersen]

On Mon, Nov 17, 2014 at 7:54 PM, Greg KH <gregkh at linuxfoundation.org> wrote:
> On Tue, Nov 18, 2014 at 12:21:29AM +0530, Susant Sahani wrote:
>> On 11/18/2014 12:06 AM, Greg KH wrote:
>> >On Mon, Nov 17, 2014 at 06:47:33PM +0100, Ronny Chevalier wrote:
>> >>2014-11-17 18:31 GMT+01:00 Greg KH <gregkh at linuxfoundation.org>:
>> >>>On Mon, Nov 17, 2014 at 10:44:14PM +0530, Susant Sahani wrote:
>> >>>>On 11/17/2014 10:39 PM, Greg KH wrote:
>> >>>>>On Mon, Nov 17, 2014 at 10:36:53PM +0530, Susant Sahani wrote:
>> >>>>>>On 11/17/2014 10:26 PM, Greg KH wrote:
>> >>>>>>>On Mon, Nov 17, 2014 at 04:28:58PM +0530, Susant Sahani wrote:
>> >>>>>>>>---
>> >>>>>>>>  src/tty-ask-password-agent/tty-ask-password-agent.c | 2 +-
>> >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>>>>
>> >>>>>>>>diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c
>> >>>>>>>>index e6dc84b..1fc792b 100644
>> >>>>>>>>--- a/src/tty-ask-password-agent/tty-ask-password-agent.c
>> >>>>>>>>+++ b/src/tty-ask-password-agent/tty-ask-password-agent.c
>> >>>>>>>>@@ -376,8 +376,8 @@ static int wall_tty_block(void) {
>> >>>>>>>>                  return -ENOMEM;
>> >>>>>>>>
>> >>>>>>>>          mkdir_parents_label(p, 0700);
>> >>>>>>>>-        mkfifo(p, 0600);
>> >>>>>>>>
>> >>>>>>>>+        (void)mkfifo(p, 0600);
>> >>>>>>>
>> >>>>>>>You really aren't "fixing" anything in these patches, just merely
>> >>>>>>>papering over the Coverity issues.  Which is fine, if you really want to
>> >>>>>>>do that, but don't think it's anything other than that...
>> >>>>>>
>> >>>>>>Yes my intention is to for coverity only Any way next line 'open' handling
>> >>>>>>the error case .
>> >>>>>
>> >>>>>I'm sorry, but I don't understand this sentance at all, can you rephrase
>> >>>>>it?
>> >>>>>
>> >>>>
>> >>>>Sorry let me rephrase it. This patch only for coverity . The next like of
>> >>>>mkfifo is open .
>> >>>>
>> >>>>(void)mkfifo(p, 0600);
>> >>>>fd = open(p, O_RDONLY|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
>> >>>>if (fd < 0)
>> >>>>         return -errno;
>> >>>>
>> >>>>and open is handling the failure.
>> >>>
>> >>>Then coverity should be fixed, don't paper over stupid bugs in tools for
>> >>>no reason.
>> >>I disagree.
>> >>
>> >>Coverity can not infer this in any possible way. How can coverity
>> >>infer that we do not care about the return value of mkfifo ?
>> >>It really depends of the semantic here.
>> >
>> >Coverity is a "semantic checker", why can't it be changed to determine
>> >if mkfifo() is followed by open() and an error check, that it is safe
>> >code?  It does this for lots of other common patterns.
>>
>> For now mkfifo/mkdir/ioctl coverity is not that smart or is it ?
>
> Talk to the coverity people.  Given that it is a closed source tool,
> that costs money, I am very loath to do anything to make it "better",
> and I really don't like it forcing programs to work around its
> deficiencies.
>
> greg k-h

What coverity is complaining about in this CID is this:
"Unchecked return value from library. Calling mkfifo() without
checking return value. This library function may fail and return an
error code."

We can choose to either make it explicit that we don't care about the
return value with this patch, or we can just mark the issue as
intentional in coverity to make it go away. The (void) is a bit ugly
imo but it does show that ignoring the return was a conscious
decision. A matter of taste I guess.

- Thomas