[PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
Hello,

I've worked on a patchset to turn on:

 -Werror=strict-overflow -Wstrict-overflow=2


The attached patchset fixes all the issues found with the compiler warning
turned on. The first part is pretty obvious int -> size_t but then there are
were some harder nuts to crack. I hope I got all right but a careful review
would be great!

I'm also fine opening a bug and backport them if someones thinks it is worth
to have it in 4.7.


Review much appreciated.


Thanks,


        Andreas

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

strict_overflow.patch1.txt (51K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
This change is wrong. Original code '(nread == 0) || ...' is more correct.
More rigorous would be (nread == 0) || (nread > 0 && namebuf[nread-1] !=
'\0').
nread is 0 resulting in invalid access to namebuf[nread-1] in while
condition check.

On 12/7/2017 2:37 PM, Andreas Schneider via samba-technical wrote:

> diff --git a/source3/modules/vfs_preopen.c b/source3/modules/vfs_preopen.c
> index 18b01e6c05e..4ef0a2e34ab 100644
> --- a/source3/modules/vfs_preopen.c
> +++ b/source3/modules/vfs_preopen.c
> @@ -156,7 +156,7 @@ static bool preopen_helper_open_one(int sock_fd, char **pnamebuf,
>  
>   nread = 0;
>  
> - while ((nread == 0) || (namebuf[nread-1] != '\0')) {
> + while (namebuf[nread-1] != '\0') {
>   ssize_t thistime;
>  
>   thistime = read(sock_fd, namebuf + nread,
> -- 2.15.1


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
On Thursday, 7 December 2017 21:18:54 CET jim via samba-technical wrote:
> This change is wrong. Original code '(nread == 0) || ...' is more correct.
> More rigorous would be (nread == 0) || (nread > 0 && namebuf[nread-1] !=
> '\0').
> nread is 0 resulting in invalid access to namebuf[nread-1] in while
> condition check.

I think we need a do { } while(namebuf[nread-1] != '\0') loop here. This would
fix it correctly.

Do you agree?

>
> On 12/7/2017 2:37 PM, Andreas Schneider via samba-technical wrote:
> > diff --git a/source3/modules/vfs_preopen.c b/source3/modules/vfs_preopen.c
> > index 18b01e6c05e..4ef0a2e34ab 100644
> > --- a/source3/modules/vfs_preopen.c
> > +++ b/source3/modules/vfs_preopen.c
> > @@ -156,7 +156,7 @@ static bool preopen_helper_open_one(int sock_fd, char
> > **pnamebuf,>
> >   nread = 0;
> >
> > - while ((nread == 0) || (namebuf[nread-1] != '\0')) {
> > + while (namebuf[nread-1] != '\0') {
> >
> >   ssize_t thistime;
> >  
> >   thistime = read(sock_fd, namebuf + nread,
> >
> > -- 2.15.1



Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
Yes, that would be much better.

On 12/7/2017 4:13 PM, Andreas Schneider wrote:

> On Thursday, 7 December 2017 21:18:54 CET jim via samba-technical wrote:
>> This change is wrong. Original code '(nread == 0) || ...' is more correct.
>> More rigorous would be (nread == 0) || (nread > 0 && namebuf[nread-1] !=
>> '\0').
>> nread is 0 resulting in invalid access to namebuf[nread-1] in while
>> condition check.
> I think we need a do { } while(namebuf[nread-1] != '\0') loop here. This would
> fix it correctly.
>
> Do you agree?
>
>> On 12/7/2017 2:37 PM, Andreas Schneider via samba-technical wrote:
>>> diff --git a/source3/modules/vfs_preopen.c b/source3/modules/vfs_preopen.c
>>> index 18b01e6c05e..4ef0a2e34ab 100644
>>> --- a/source3/modules/vfs_preopen.c
>>> +++ b/source3/modules/vfs_preopen.c
>>> @@ -156,7 +156,7 @@ static bool preopen_helper_open_one(int sock_fd, char
>>> **pnamebuf,>
>>>     nread = 0;
>>>
>>> - while ((nread == 0) || (namebuf[nread-1] != '\0')) {
>>> + while (namebuf[nread-1] != '\0') {
>>>
>>>     ssize_t thistime;
>>>    
>>>     thistime = read(sock_fd, namebuf + nread,
>>>
>>> -- 2.15.1
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, Dec 07, 2017 at 08:37:15PM +0100, Andreas Schneider via samba-technical wrote:

> From 9b5684f9acac4c4ff0136c0ef9663d8f9f4c28bf Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Thu, 26 Oct 2017 09:43:56 +0200
> Subject: [PATCH 42/42] s3:glock: Add sanity check in g_lock_parse()
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> ---
>  source3/lib/g_lock.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
> index c5d66e3855e..ea2d7717ce5 100644
> --- a/source3/lib/g_lock.c
> +++ b/source3/lib/g_lock.c
> @@ -81,6 +81,9 @@ static bool g_lock_parse(uint8_t *buf, size_t buflen, struct g_lock *lck)
>   buf += sizeof(uint32_t);
>   buflen -= sizeof(uint32_t);
>   data_ofs = found_recs * G_LOCK_REC_LENGTH;
> + if (data_ofs >= buflen) {
> + return false;
> + }

Can we better fix this by moving the

        if (found_recs > buflen/G_LOCK_REC_LENGTH) {
                return false;
        }

sequence to after the buflen-=sizeof(uint32_t)?

I thought that overflow in multiplication is much better caught by
testing the reverse operation before the multiplication is done.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
On Friday, 8 December 2017 08:41:08 CET Volker Lendecke wrote:
> On Thu, Dec 07, 2017 at 08:37:15PM +0100, Andreas Schneider via samba-
technical wrote:

> > From 9b5684f9acac4c4ff0136c0ef9663d8f9f4c28bf Mon Sep 17 00:00:00 2001
> > From: Andreas Schneider <[hidden email]>
> > Date: Thu, 26 Oct 2017 09:43:56 +0200
> > Subject: [PATCH 42/42] s3:glock: Add sanity check in g_lock_parse()
> >
> > Signed-off-by: Andreas Schneider <[hidden email]>
> > ---
> >
> >  source3/lib/g_lock.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
> > index c5d66e3855e..ea2d7717ce5 100644
> > --- a/source3/lib/g_lock.c
> > +++ b/source3/lib/g_lock.c
> > @@ -81,6 +81,9 @@ static bool g_lock_parse(uint8_t *buf, size_t buflen,
> > struct g_lock *lck)>
> >   buf += sizeof(uint32_t);
> >   buflen -= sizeof(uint32_t);
> >   data_ofs = found_recs * G_LOCK_REC_LENGTH;
> >
> > + if (data_ofs >= buflen) {
> > + return false;
> > + }
>
> Can we better fix this by moving the
>
>         if (found_recs > buflen/G_LOCK_REC_LENGTH) {
>                 return false;
>         }
>
> sequence to after the buflen-=sizeof(uint32_t)?
>
> I thought that overflow in multiplication is much better caught by
> testing the reverse operation before the multiplication is done.
Yes, that's better. Updated patchset attached.


Thanks,


        Andreas

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

strict_overflow.patch2.txt (49K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thursday, 7 December 2017 20:37:15 CET Andreas Schneider via samba-
technical wrote:

> Hello,
>
> I've worked on a patchset to turn on:
>
>  -Werror=strict-overflow -Wstrict-overflow=2
>
>
> The attached patchset fixes all the issues found with the compiler warning
> turned on. The first part is pretty obvious int -> size_t but then there are
> were some harder nuts to crack. I hope I got all right but a careful review
> would be great!
>
> I'm also fine opening a bug and backport them if someones thinks it is worth
> to have it in 4.7.
Volker only pushed one patch from the patchset, attached is a rebased new
patchset.


Review much appreciated.
 
 
Thanks,
 
 
  Andreas


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

strict_overflow.patch3.txt (49K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHSET] Fix and add -Werror=strict-overflow -Wstrict-overflow=2

Samba - samba-technical mailing list
On Tuesday, 12 December 2017 08:34:19 CET Andreas Schneider via samba-
technical wrote:

> On Thursday, 7 December 2017 20:37:15 CET Andreas Schneider via samba-
>
> technical wrote:
> > Hello,
> >
> > I've worked on a patchset to turn on:
> >  -Werror=strict-overflow -Wstrict-overflow=2
> >
> > The attached patchset fixes all the issues found with the compiler warning
> > turned on. The first part is pretty obvious int -> size_t but then there
> > are were some harder nuts to crack. I hope I got all right but a careful
> > review would be great!
> >
> > I'm also fine opening a bug and backport them if someones thinks it is
> > worth to have it in 4.7.
>
> Volker only pushed one patch from the patchset, attached is a rebased new
> patchset.
>

Happy new year ping!

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