Quantcast

[PATCH] Address several issues in Samba

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] Address several issues in Samba

Andreas Schneider-15
Hello,

I'm currently packaging Samba for the next RHEL release. Our covscan tool
found a lot of issues. I would like to address some. The atttached patchset
fixes 12 issues and adds a modeling file for coverity we can upload to tell it
how tests are working. It can be extended and uploaded. Currently it is only
for cmocka tests.

Review and push appreciated.


Thanks,


        Andreas



--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

fixes.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Address several issues in Samba

Jeremy Allison
On Fri, Feb 17, 2017 at 12:09:36PM +0100, Andreas Schneider wrote:
> Hello,
>
> I'm currently packaging Samba for the next RHEL release. Our covscan tool
> found a lot of issues. I would like to address some. The atttached patchset
> fixes 12 issues and adds a modeling file for coverity we can upload to tell it
> how tests are working. It can be extended and uploaded. Currently it is only
> for cmocka tests.
>
> Review and push appreciated.

+1 on all of these except:

-----------------------------------------------------------------------------------
 From 9fe8650643bd9bc5657d52d2507b3b76ef15def7 Mon Sep 17 00:00:00 2001
 From: Andreas Schneider <[hidden email]>
 Date: Fri, 17 Feb 2017 10:04:14 +0100
 Subject: [PATCH 08/13] s3:winbind: Add a paranoia check that result is not
  NULL
 
 Found by covscan.
 
 BUG: https://bugzilla.samba.org/show_bug.cgi?id=12592
 
 Signed-off-by: Andreas Schneider <[hidden email]>
 ---
  source3/winbindd/winbindd_list_users.c | 3 +++
  1 file changed, 3 insertions(+)
 
 diff --git a/source3/winbindd/winbindd_list_users.c b/source3/winbindd/winbindd_list_users.c
 index 9a751a75c5b..aa76b3c4f79 100644
 --- a/source3/winbindd/winbindd_list_users.c
 +++ b/source3/winbindd/winbindd_list_users.c
 @@ -171,6 +171,9 @@ NTSTATUS winbindd_list_users_recv(struct tevent_req *req,
                       return map_nt_error_from_unix(ret);
               }
       }
 +     if (result == NULL) {
 +             return NT_STATUS_INTERNAL_ERROR;
 +     }
 
       len = talloc_get_size(result);
-----------------------------------------------------------------------------------

Can you share the coverity error message on this one ?

Returning response->data.num_entries == 0 with
NT_STATUS_OK (which is what you'd get if result == NULL)
looks OK to me.

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Address several issues in Samba

Andreas Schneider-15
On Friday, 17 February 2017 23:45:02 CET Jeremy Allison wrote:

> On Fri, Feb 17, 2017 at 12:09:36PM +0100, Andreas Schneider wrote:
> > Hello,
> >
> > I'm currently packaging Samba for the next RHEL release. Our covscan tool
> > found a lot of issues. I would like to address some. The atttached
> > patchset
> > fixes 12 issues and adds a modeling file for coverity we can upload to
> > tell it how tests are working. It can be extended and uploaded. Currently
> > it is only for cmocka tests.
> >
> > Review and push appreciated.
>
> +1 on all of these except:
>
> ----------------------------------------------------------------------------
> ------- From 9fe8650643bd9bc5657d52d2507b3b76ef15def7 Mon Sep 17 00:00:00
> 2001 From: Andreas Schneider <[hidden email]>
>  Date: Fri, 17 Feb 2017 10:04:14 +0100
>  Subject: [PATCH 08/13] s3:winbind: Add a paranoia check that result is not
>   NULL
>
>  Found by covscan.
>
>  BUG: https://bugzilla.samba.org/show_bug.cgi?id=12592
>
>  Signed-off-by: Andreas Schneider <[hidden email]>
>  ---
>   source3/winbindd/winbindd_list_users.c | 3 +++
>   1 file changed, 3 insertions(+)
>
>  diff --git a/source3/winbindd/winbindd_list_users.c
> b/source3/winbindd/winbindd_list_users.c index 9a751a75c5b..aa76b3c4f79
> 100644
>  --- a/source3/winbindd/winbindd_list_users.c
>  +++ b/source3/winbindd/winbindd_list_users.c
>  @@ -171,6 +171,9 @@ NTSTATUS winbindd_list_users_recv(struct tevent_req
> *req, return map_nt_error_from_unix(ret);
>                }
>        }
>  +     if (result == NULL) {
>  +             return NT_STATUS_INTERNAL_ERROR;
>  +     }
>
>        len = talloc_get_size(result);
> ----------------------------------------------------------------------------
> -------
>
> Can you share the coverity error message on this one ?
>
> Returning response->data.num_entries == 0 with
> NT_STATUS_OK (which is what you'd get if result == NULL)
> looks OK to me.

2. Defect type: FORWARD_NULL
3. samba-4.6.0rc3/source3/winbindd/winbindd_list_users.c:159: assign_zero:
Assigning: "result" = "NULL".
16. samba-4.6.0rc3/source3/winbindd/winbindd_list_users.c:186: var_deref_op:
Dereferencing null pointer "result".
#   184|  
#   185|   for (i=0; i<len; i++) {
#   186|-> if (result[i] == '\0') {
#   187|   result[i] = ',';
#   188|   response->data.num_entries += 1;



--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Address several issues in Samba

Jeremy Allison
On Thu, Feb 23, 2017 at 02:26:06PM +0100, Andreas Schneider wrote:

> On Friday, 17 February 2017 23:45:02 CET Jeremy Allison wrote:
> > On Fri, Feb 17, 2017 at 12:09:36PM +0100, Andreas Schneider wrote:
> > > Hello,
> > >
> > > I'm currently packaging Samba for the next RHEL release. Our covscan tool
> > > found a lot of issues. I would like to address some. The atttached
> > > patchset
> > > fixes 12 issues and adds a modeling file for coverity we can upload to
> > > tell it how tests are working. It can be extended and uploaded. Currently
> > > it is only for cmocka tests.
> > >
> > > Review and push appreciated.
> >
> > +1 on all of these except:
> >
> > ----------------------------------------------------------------------------
> > ------- From 9fe8650643bd9bc5657d52d2507b3b76ef15def7 Mon Sep 17 00:00:00
> > 2001 From: Andreas Schneider <[hidden email]>
> >  Date: Fri, 17 Feb 2017 10:04:14 +0100
> >  Subject: [PATCH 08/13] s3:winbind: Add a paranoia check that result is not
> >   NULL
> >
> >  Found by covscan.
> >
> >  BUG: https://bugzilla.samba.org/show_bug.cgi?id=12592
> >
> >  Signed-off-by: Andreas Schneider <[hidden email]>
> >  ---
> >   source3/winbindd/winbindd_list_users.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> >  diff --git a/source3/winbindd/winbindd_list_users.c
> > b/source3/winbindd/winbindd_list_users.c index 9a751a75c5b..aa76b3c4f79
> > 100644
> >  --- a/source3/winbindd/winbindd_list_users.c
> >  +++ b/source3/winbindd/winbindd_list_users.c
> >  @@ -171,6 +171,9 @@ NTSTATUS winbindd_list_users_recv(struct tevent_req
> > *req, return map_nt_error_from_unix(ret);
> >                }
> >        }
> >  +     if (result == NULL) {
> >  +             return NT_STATUS_INTERNAL_ERROR;
> >  +     }
> >
> >        len = talloc_get_size(result);
> > ----------------------------------------------------------------------------
> > -------
> >
> > Can you share the coverity error message on this one ?
> >
> > Returning response->data.num_entries == 0 with
> > NT_STATUS_OK (which is what you'd get if result == NULL)
> > looks OK to me.
>
> 2. Defect type: FORWARD_NULL
> 3. samba-4.6.0rc3/source3/winbindd/winbindd_list_users.c:159: assign_zero:
> Assigning: "result" = "NULL".
> 16. samba-4.6.0rc3/source3/winbindd/winbindd_list_users.c:186: var_deref_op:
> Dereferencing null pointer "result".
Ah, it's a false positive. If result == NULL
then:

175         len = talloc_get_size(result);

will return len = 0, so we can't get to the
problematic code.

If you want to make the false positive message
go away, try the following patch. This doesn't
change the semantics of winbindd_list_users_recv().

Cheers,

        Jeremy.

0001-s3-winbind-work-around-coverity-false-positive.patch (912 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] Address several issues in Samba

Andreas Schneider-15
On Thursday, 23 February 2017 18:46:09 CET Jeremy Allison wrote:

> Ah, it's a false positive. If result == NULL
> then:
>
> 175         len = talloc_get_size(result);
>
> will return len = 0, so we can't get to the
> problematic code.
>
> If you want to make the false positive message
> go away, try the following patch. This doesn't
> change the semantics of winbindd_list_users_recv().
>
> Cheers,
>
> Jeremy.

Ok, we can discard it or you push it. RB+


--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Loading...