bugdmidecode - Bugs: bug #48723, dmidecode.c:2283: code in wrong...

 
 

bug #48723: dmidecode.c:2283: code in wrong order ?

Submitter:  David Binderman <dcb314>
Submitted:  Mon 08 Aug 2016 04:10:35 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  khali
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 07 Sep 2016 08:19:43 PM UTC, comment #6: 

"Fixed" (although there was no actual bug):

Clarify a comment in dmi_memory_device_extended_size
http://git.savannah.gnu.org/cgit/dmidecode.git/commit/?id=5696fa33e5f9fce843fa3b5972b87a705fed2067

Prevent static code analyzer confusion
http://git.savannah.gnu.org/cgit/dmidecode.git/commit/?id=0b5c47c64b6fb3b626c5f75d9dc36bc864907fca

Jean Delvare <khali>
Group administrator
Wed 07 Sep 2016 08:08:57 PM UTC, comment #5: 

I can't say I'm impressed by your comments, sorry. You are basically translating from C to English, line by line. This if #1 in what not to do when adding comments to code:

https://improvingsoftware.com/2011/06/27/5-best-practices-for-commenting-your-code/

Comments should point out what is not obvious from reading the code. It is not meant for people who do not understand the programming language being used.

So I'll stick to my version, sorry ^^

Jean Delvare <khali>
Group administrator
Mon 29 Aug 2016 08:42:22 PM UTC, comment #4: 


>please tell me if it is clear enough for you.


Clear as mud to me, I am sorry to say.

My best effort is this:

    if (code & 0x3FFUL)
    {
        /* code has some less significant bits set */
        printf(" %lu MB", (unsigned long)code);
    }
    else if (code & 0xFFC00UL)
    {
        /* code has some bits in the middle set */
        printf(" %lu GB", (unsigned long)code >> 10);
    }
    else
    {
        /* code has clear bits in the middle and less significant areas */
        printf(" %lu TB", (unsigned long)code >> 20);
    }

> I could turn 0xFFFFFUL into 0xFFC00UL to make the static code > analyzer happy (I think - can you confirm?)


Yes, this change makes it happy.

It also looks like clearer code to me, too.

David Binderman <dcb314>
Mon 29 Aug 2016 04:39:18 PM UTC, comment #3: 

Patch attached, please tell me if the it is clear enough for you. If not, you are welcome to propose your own wording.

You should discuss the problem with whoever maintains the static code analyzer you used. Its claim is incorrect. "Expression is always false" is not a proper description of the problem. There is an overlap of the conditions, but the second condition is broader, so it can still match where the first condition didn't.

I see how my code looks suspicious. If "code" was a bitfield, then the code would indeed possibly be wrong. And the performed tests make it look like a bitfield. I could turn 0xFFFFFUL into 0xFFC00UL to make the static code analyzer happy (I think - can you confirm?) The output of dmidecode would be exactly the same.

So a static code analyzer could report the situation as "suspicious overlapping bitmasks" or something like that. But not as a condition which will never match.

Jean Delvare <khali>
Group administrator
Sat 27 Aug 2016 10:58:52 AM UTC, comment #2: 


>Is this the output of some static code analyzer?


Yes.

>The logic of the code is that the displayed value is never >rounded. So it doesn't test whether value is large enough to be >displayed as TB or GB (is that was the intent, we would indeed >run the tests the other way around), instead it tests that value >can be displayed as TB or GB without losing the least significant >bits. So the order is correct.


Thanks for the explanation. The code now looks ok to me.

There might be some value in adding a comment, to explain
the code better, so future readers don't make the
same mistake I did.

David Binderman <dcb314>
Sat 27 Aug 2016 10:34:48 AM UTC, comment #1: 

Is this the output of some static code analyzer?

I can't see any bug in this function. If "code" is 0x01000, the first condition will not match (0x01000 & 0x003FF is 0 == false), but the second condition will match (0x01000 & 0xFFFFF is 0x01000 == true), as intended.

The logic of the code is that the displayed value is never rounded. So it doesn't test whether value is large enough to be displayed as TB or GB (is that was the intent, we would indeed run the tests the other way around), instead it tests that value can be displayed as TB or GB without losing the least significant bits. So the order is correct.

Jean Delvare <khali>
Group administrator
Mon 08 Aug 2016 04:10:35 PM UTC, original submission:  

dmidecode.c:2283]: (style) Expression is always false because 'else if' condition matches previous condition at line 2281.

Source code is

    /* Use the most suitable unit depending on size */
    if (code & 0x3FFUL)
        printf(" %lu MB", (unsigned long)code);
    else if (code & 0xFFFFFUL)
        printf(" %lu GB", (unsigned long)code >> 10);
    else
        printf(" %lu TB", (unsigned long)code >> 20);

Surely it is best to test for TB first, then if none,
test for GB and then if none, default to MB ?


David Binderman <dcb314>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #38382:  dmidecode-clarify-unit-choice.patch added by khali (758B - text/x-patch - [PATCH] Clarify a comment in dmi_memory_device_extended_size)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by dcb314 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-09-07 khali StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2016-08-29 khali Attached File- Added dmidecode-clarify-unit-choice.patch, #38382
    2016-08-27 khali StatusNone Need Info
        Assigned toNone khali

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code