Device list sort function bug

Powertop Igor Zhbanov 5 participants 7 messages Jul 11, 2012 – Jul 17, 2012

Share & tools

Igor Zhbanov

author

2012-07-11 06:11 UTC #0 lore raw

Hello!

Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.

The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.

Here is the patch:
--8<--Cut-here----------------------------------------------------------------

src/devlist.cpp +1 -1

diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp
@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser
  	if (i->pid != j->pid)
  		return i->pid<  j->pid;
 
-	return strcmp(i->device, j->device);
+	return (strcmp(i->device, j->device)<  0);
  }
 
  static const char *dev_class(int line)

--8<--------------------------------------------------------------------------

Thank you.

Signature · 9 lines

Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

src/devlist.cpp +1 -1

diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp
@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser
 	if (i->pid != j->pid)
 		return i->pid < j->pid;
 
-	return strcmp(i->device, j->device);
+	return (strcmp(i->device, j->device) < 0);
 }
 
 static const char *dev_class(int line)

Sergey Senozhatsky

2012-07-11 07:33 UTC #1 lore raw

Quoted 11 lines · Igor Zhbanov

Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.

Jan, could you please test this one?

Thanks in advance,

-ss

Paul Menzel

2012-07-11 07:52 UTC #5 lore raw

In reply to Igor Zhbanov

Dear Igor,

thank you for your patches!

Am Mittwoch, den 11.07.2012, 10:11 +0400 schrieb Igor Zhbanov:

Quoted 16 lines

Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.
Here is the patch:
--8<--Cut-here---------------------------------------------------------------- diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp

src/devlist.cpp static bool devlist_sort(struct devuser

Quoted 9 lines

src/devlist.cpp +1 -2

@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser
  	if (i->pid != j->pid)
  		return i->pid<  j->pid;
 
-	return strcmp(i->device, j->device);
+	return (strcmp(i->device, j->device)<  0);
  }
 
  static const char *dev_class(int line)
--8<--------------------------------------------------------------------------

Nice find!

To decrease work for the maintainers, could you please send patches formatted by git format-patch? After that you can either use git send-email or copy the output into Seamonkey’s new message window.

        $ git clone git://github.com/fenrus75/powertop.git
        $ cd powertop
        $ git config --global user.name "Igor Zhbanov"
        $ git config --global user.email i.zhbanov(a)samsung.com
        $ git checkout -b sort-fix # new branch for each fix
        $ # apply your change
        $ git commit -a # enter a nice commit message \
        you should be able to use your great ones from your messages
        $ git format-patch -1

Then maintainers can just apply it using the following command.

    $ git am yourmessagewithpatch.mbox

If you need more help with Git, just ask.

Thanks,

Paul

Jan-Simon Möller <dl9pf at gmx.de>

2012-07-11 07:55 UTC #2 lore raw

In reply to Sergey Senozhatsky

Yes, in an hour or so.
Best,
JS
Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:

Quoted 16 lines · Igor Zhbanov

Quoted 9 lines
Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.
Jan, could you please test this one?
Thanks in advance,
	-ss

Igor Zhbanov

author

2012-07-11 08:16 UTC #3 lore raw

So here is the formatted version:

From 5d284137a6035bab65717fbacd98423b96d862cf Mon Sep 17 00:00:00 2001
From: Igor Zhbanov<i.zhbanov(a)samsung.com>
Date: Wed, 11 Jul 2012 12:08:21 +0400
Subject: [PATCH] Device list sort function bug

---
  src/devlist.cpp |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

src/devlist.cpp +1 -1

index 93f2081..cd5b5d8 100644
@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser * i, struct devuser * j)
  	if (i->pid != j->pid)
  		return i->pid<  j->pid;
 
-	return strcmp(i->device, j->device);
+	return (strcmp(i->device, j->device)<  0);
  }
 
  static const char *dev_class(int line)

Signature · 1 line

1.7.5.4

Quoted 19 lines · Jan-Simon Möller

Yes, in an hour or so.
Best,
JS
Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
Quoted 15 lines · Igor Zhbanov
Quoted 9 lines
Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.
Jan, could you please test this one?
Thanks in advance,
	-ss

Signature · 9 lines

Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Paul Menzel

2012-07-11 08:57 UTC #4 lore raw

Dear Igor,

thank you for following my advise. Some more minor suggestions are inlined but until the maintainers ask for further changes you do not need to take any further actions.

Am Mittwoch, den 11.07.2012, 12:16 +0400 schrieb Igor Zhbanov:

Quoted 1 line

So here is the formatted version:

Normally when sending patches no introduction or email style formalities (like greeting and so on) are needed.

If you want to use them put them at the top and separate them from the patch using

---- 8< ----- >8 ----

so that git am --scissors strips the email text off automatically.

Quoted 2 lines

From 5d284137a6035bab65717fbacd98423b96d862cf Mon Sep 17 00:00:00 2001
From: Igor Zhbanov<i.zhbanov(a)samsung.com>

SeaMonkey and some Thunderbird versions seem to have a bug deleting a space before »<«.

But if your email account you sending the message from is the same as the commit author of the patch you can also delete that line.

Quoted 2 lines

Date: Wed, 11 Jul 2012 12:08:21 +0400
Subject: [PATCH] Device list sort function bug

You can copy the text after »Subject: « into the email message subject line. Then your message automatically has the »PATCH« tag.

Quoted 10 lines

Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.
---

Some comments not meant for the commit message, like what changed in patch iterations go after the --- line and Git will ignore them.

Quoted 18 lines

  src/devlist.cpp |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

src/devlist.cpp +1 -1

index 93f2081..cd5b5d8 100644
@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser * i, struct devuser * j)
  	if (i->pid != j->pid)
  		return i->pid<  j->pid;
 
-	return strcmp(i->device, j->device);
+	return (strcmp(i->device, j->device)<  0);
  }
 
  static const char *dev_class(int line)

Signature · 1 line

1.7.5.4

Just delete the quote next time if you are not citing it in any way [1].

Quoted 15 lines

Quoted 14 lines · Jan-Simon Möller
Yes, in an hour or so.
Best,
JS
Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
Quoted 10 lines · Igor Zhbanov
Quoted 9 lines
Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort() function and it crosses boundary of array.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Jan, could you please test this one?
Thanks in advance,
	-ss

Thanks again,

Paul

[1] http://en.opensuse.org/openSUSE:Mailing_list_netiquette

Chris Ferron

2012-07-17 22:09 UTC #6 lore raw

In reply to Igor Zhbanov

Quoted 32 lines · Igor Zhbanov

Hello!
Sometimes PowerTOP dies with a Segmentation Fault while generating the report. Little investigation shown that it dies while sorting the device list. The problem is that comparison function devlist_sort is incorrect.
The function should return true if first argument "preceeds" second. But when strcmp() is used, it can return -1, 0 and 1. So both -1 and 1 values are silently converted to true, which is wrong. It confuses the sort()
function and it crosses boundary of array.
Here is the patch:
--8<--Cut-here----------------------------------------------------------------
diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp
src/devlist.cpp 2012-07-03 20:01:05.896232845 renamed from src/devlist.cpp 2012-07-03 17:00:50.000000000 +0400 +2 -2
+0400
@@ -273,7 +273,7 @@ static bool devlist_sort(struct devuser
     if (i->pid != j->pid)
         return i->pid<  j->pid;
 
-    return strcmp(i->device, j->device);
+    return (strcmp(i->device, j->device)<  0);
 }
 
 static const char *dev_class(int line)
--8<-------------------------------------------------------------------------- 

Quoted 8 lines

Thank you.
_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

Your patch has been merged.
Thank you,