Re: [Powertop] Device list sort function bug

Re: [Powertop] Device list sort function bug

[@ 2012-07-11 14:43 UTC (permalink / raw)]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
> On (07/11/12 10:11), Igor Zhbanov wrote:
> > 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

Confirmed. Fixes the crash & works here.

Acked-by: Jan-Simon Moeller <dl9pf(a)gmx.de>

Re: [Powertop] Device list sort function bug

[Chris Ferron]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On 07/10/2012 11:11 PM, Igor Zhbanov wrote:
> 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
> --- powertop-il/src/devlist.cpp    2012-07-03 17:00:50.000000000 +0400
> +++ powertop-il-fix/src/devlist.cpp    2012-07-03 20:01:05.896232845 
> +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<-------------------------------------------------------------------------- 
>
>
> Thank you.
>
>
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
Your patch has been merged.
Thank you,

Re: [Powertop] Device list sort function bug

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]

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:
> 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.

>  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.

> 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.

> 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.

>   src/devlist.cpp |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/devlist.cpp b/src/devlist.cpp
> index 93f2081..cd5b5d8 100644
> --- a/src/devlist.cpp
> +++ b/src/devlist.cpp
> @@ -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)
> -- 
> 1.7.5.4

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

> Jan-Simon Möller wrote:
> > Yes, in an hour or so.
> > Best,
> > JS
> > Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
> >> On (07/11/12 10:11), Igor Zhbanov wrote:
> >>> 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


Thanks again,

Paul


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Powertop] Device list sort function bug

[Igor Zhbanov]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]

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

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.
---
  src/devlist.cpp |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/devlist.cpp b/src/devlist.cpp
index 93f2081..cd5b5d8 100644
--- a/src/devlist.cpp
+++ b/src/devlist.cpp
@@ -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)
-- 
1.7.5.4

Jan-Simon Möller wrote:
> Yes, in an hour or so.
> Best,
> JS
> Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
>> On (07/11/12 10:11), Igor Zhbanov wrote:
>>> 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
-- 
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

Re: [Powertop] Device list sort function bug

[@ 2012-07-11 7:55 UTC (permalink / raw)]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Yes, in an hour or so.
Best,
JS
Am Mittwoch, 11. Juli 2012, 09:33:42 schrieb Sergey Senozhatsky:
> On (07/11/12 10:11), Igor Zhbanov wrote:
> > 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

Re: [Powertop] Device list sort function bug

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

Dear Igor,


thank you for your patches!


Am Mittwoch, den 11.07.2012, 10:11 +0400 schrieb 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.
> 
> Here is the patch:
> --8<--Cut-here----------------------------------------------------------------
> diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp
> --- powertop-il/src/devlist.cpp	2012-07-03 17:00:50.000000000 +0400
> +++ powertop-il-fix/src/devlist.cpp	2012-07-03 20:01:05.896232845 +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<--------------------------------------------------------------------------

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Powertop] Device list sort function bug

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On (07/11/12 10:11), Igor Zhbanov wrote:
> 
> 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

[Powertop] Device list sort function bug

[Igor Zhbanov]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

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
--- powertop-il/src/devlist.cpp	2012-07-03 17:00:50.000000000 +0400
+++ powertop-il-fix/src/devlist.cpp	2012-07-03 20:01:05.896232845 +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<--------------------------------------------------------------------------

Thank you.

-- 
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 03-powertop-fix01.diff --]
[-- Type: text/x-diff, Size: 453 bytes --]

diff -purN powertop-il/src/devlist.cpp powertop-il-fix/src/devlist.cpp
--- powertop-il/src/devlist.cpp	2012-07-03 17:00:50.000000000 +0400
+++ powertop-il-fix/src/devlist.cpp	2012-07-03 20:01:05.896232845 +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)