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

 
 

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

Submitted by:  David Binderman <dcb314>
Submitted on:  Mon 08 Aug 2016 04:10:35 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Jean Delvare <khali>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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 File(s):
   
   
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
  • -unavailable- added by khali (Posted a comment)
  • -unavailable- added by dcb314 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 07 Sep 2016 08:19:43 PM UTCkhaliStatusNeed Info=>Fixed
      Open/ClosedOpen=>Closed
    Mon 29 Aug 2016 04:21:24 PM UTCkhaliAttached File-=>Added dmidecode-clarify-unit-choice.patch, #38382
    Sat 27 Aug 2016 10:34:48 AM UTCkhaliStatusNone=>Need Info
      Assigned toNone=>khali

    Back to the top


    Powered by Savane 3.1-cleanup1