[PATCH] Enforce strict overflow checking

[Stefan Metzmacher]

Am 03.04.2018 um 19:23 schrieb Jeremy Allison via samba-technical:
> On Tue, Apr 03, 2018 at 06:10:29PM +0200, Andreas Schneider wrote:
>> On Tuesday, 3 April 2018 17:45:25 CEST Jeremy Allison wrote:
>>> On Tue, Apr 03, 2018 at 02:02:53PM +0200, Andreas Schneider wrote:
>>>> Ah, sorry my bad. I didn't have the tree with the reverted patch. Here we
>>>> go:
>>>>
>>>>
>>>> ../source3/nmbd/nmbd_incomingrequests.c: In function
>>>> ‘process_node_status_request’:
>>>> ../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
>>>> wraparound does not occur when comparing P +- C1 with P +- C2
>>>> [-Werror=strict- overflow]
>>>>
>>>>   while (buf < bufend) {
>>>>   
>>>>         ^
>>>>
>>>> cc1: all warnings being treated as errors
>>>
>>> Ah, I'm not seeing that. What version of gcc are you using ?
>>
>> I'm not seeing them either with gcc 7.3.1 on my machines!
>>
>>
>> I was only able to reproduce this on sn-devel (autobuild) which uses:
>>
>>   gcc-4.8.real (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4
>>
>>> Are there a lot of these in nmbd ? Is it worth fixing nmbd
>>> first, or set to ignore now and work on them later ?
>>
>> I don't know. It might be after fixing that one, that it will find more in 
>> that file.
>>
>> However fixing this one will probably a bit tricky. The optimizer doesn't like 
>> the pointer arithmetic. So you need to use array indices to do the overflow 
>> calculations. By fixing that we would allow the optimizer to understand the 
>> limitations better.
>>
>> It is possible that gcc 7 is cleverer than 4.8 in this regard and can deal 
>> with the pointer arithmetic here.
>>
>>
>>
>> I hope that helps.
> 
> Sure does. RB+ and pushed all except the last two patches
> that turn on the compiler shitches (or off in the nmbd
> case :-).
> 
> I would like to get Metze's buy-off before pressing the
> final big red button (tm) :-).

I would prefer that we fix the warning, this seems to do the trick
with gcc 4.8:

diff --git a/source3/nmbd/nmbd_incomingrequests.c
b/source3/nmbd/nmbd_incomingrequests.c
index 6f3eee3..96cb124 100644
--- a/source3/nmbd/nmbd_incomingrequests.c
+++ b/source3/nmbd/nmbd_incomingrequests.c
@@ -341,7 +341,7 @@ subnet %s - name not found.\n",
nmb_namestr(&nmb->question.question_name),

        namerec = subrec->namelist;

-       while (buf < bufend) {
+       while (PTR_DIFF(bufend, buf) > 0) {
                if( (namerec->data.source == SELF_NAME) ||
(namerec->data.source == PERMANENT_NAME) ) {
                        int name_type = namerec->name.name_type;
                        unstring name;


Can you double check it doesn't change the logic?

But process_node_status_request() is a complete mess and should be
rewritten by someone who understands it:-)

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180403/12067aaa/signature.sig>