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

[Ronny Chevalier]

2014-11-17 19:36 GMT+01:00 Greg KH <gregkh at linuxfoundation.org>:
> 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 me I see this as a warning, for some cases it is safe and there is
no problem like this one so we can document the code for us and tools
like Coverity, but it can be a mistake and maybe it should have been
checked. So Coverity assumes the worst case by warning us, and I don't
see the problem.

>
> greg k-h