Bug 207854 - usr/src/sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c:1437: bad shift ?
Summary: usr/src/sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c:1437: bad shift ?
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: buildisok, patch, security
Depends on:
Blocks:
 
Reported: 2016-03-09 19:27 UTC by David Binderman
Modified: 2019-12-21 11:40 UTC (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Binderman 2016-03-09 19:27:30 UTC 
[usr/src/sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c:1437]: (error) Shifting 32-bit value by 63 bits is undefined behaviour

Source code is

       for (tmpN=7 ; tmpN<64; tmpN++ )
        {
            tmp = ABS((int)(slope - tmpA/(1<<tmpN)));

Maybe better code

       for (tmpN=7 ; tmpN<64; tmpN++ )
        {
            tmp = ABS((int)(slope - tmpA / (1UL << tmpN)));
Comment 1 Gleb Popov freebsd_committer freebsd_triage 2019-10-31 15:07:18 UTC 
Not sure how UL would help here, since unsigned long is also 32-bits. The proper fix is to use tmpN modulo 32 as shift operand. Verified this on standalone example:

#include <stdio.h>
#include <sys/types.h>

int main(int argc, char* argv[])
{
    uint32_t tmpN;
    int tmp;
    for (tmpN=7 ; tmpN<64; tmpN++ )
    {
        //tmp = (1<<(tmpN%32));
        tmp = (1UL<<(tmpN%32));
        printf("%d\n", tmp);
    }
    return 0;
}


Patch for the problem:

Index: sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c
===================================================================
--- sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c     (revision 353638)
+++ sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c     (working copy)
@@ -1468,7 +1468,7 @@
     for (tmpA=(uint32_t)(64*pres) ; tmpA<128*pres; tmpA += pres )
         for (tmpN=7 ; tmpN<64; tmpN++ )
         {
-            tmp = ABS((int)(slope - tmpA/(1<<tmpN)));
+            tmp = ABS((int)(slope - tmpA/(1UL<<(tmpN%32))));
             if (tmp < gap)
             {
                sa = tmpA;
Comment 2 Gleb Popov freebsd_committer freebsd_triage 2019-11-18 18:37:53 UTC 
Bump.
Comment 3 Gleb Popov freebsd_committer freebsd_triage 2019-11-26 06:46:17 UTC 
Bump.
Comment 4 Gleb Popov freebsd_committer freebsd_triage 2019-12-03 04:52:10 UTC 
Bump.
Comment 5 David Binderman 2019-12-03 07:38:48 UTC 
I am not sure who the bump is for.

If it is for me, I gave up working on freebsd a year or two back
and so you are on your own with this one.

1 is a 32 bit value, 1UL can be a 64 bit value on some architectures.
Comment 6 Gleb Popov freebsd_committer freebsd_triage 2019-12-03 08:35:50 UTC 
(In reply to David Binderman from comment #5)

> If it is for me, I gave up working on freebsd a year or two back
and so you are on your own with this one.

For freebsd-bugs@, so some committer would pick this up and commit. You can remove yourself from CC list, I think.
Comment 7 Mark Linimon freebsd_committer freebsd_triage 2019-12-03 18:06:50 UTC 
(In reply to Gleb Popov from comment #6)
> You can remove yourself from CC list, I think.

No, he can't, as he is the originator, and not actually on the Cc: list.
Comment 8 Gleb Popov freebsd_committer freebsd_triage 2019-12-09 14:20:47 UTC 
(In reply to Mark Linimon from comment #7)
Then let's commit this trivial patch and bother reporter no more.
Comment 9 commit-hook freebsd_committer freebsd_triage 2019-12-21 11:39:43 UTC 
A commit references this bug:

Author: arrowd
Date: Sat Dec 21 11:38:48 UTC 2019
New revision: 355980
URL: https://svnweb.freebsd.org/changeset/base/355980

Log:
  Don't shift 32-bit value by more than 32 bits.

  PR:		207854
  Approved by:	emaste

Changes:
  head/sys/contrib/ncsw/Peripherals/QM/qm_portal_fqr.c