Bug 238511 – sys-power/suspend contains an overflow bug that stops it working

• Product: Gentoo Linux
• Status: RESOLVED FIXED

[Description ferret 2008-09-23 21:26:10 UTC]

When I first configured and tried s2disk, it seemed to work (splash progress bar and all) up to the point when I would expect it to power off the system.  It then prints an S and a newline and then returns to the shell.

I tracked the problem down to this C code in suspend.c and resume.c:

1388:	unsigned int size = sizeof(struct swsusp_header);
1389:	unsigned int shift = (resume_offset + 1) * page_size - size;

Since I was using a swap file, my resume_offset is 528894, page_size is 4096, and size is 28 (I have checked these values by inserting a printf in the above code):

$ printf "%X\n" $(( (5228894 + 1) * 4096 - 28 ))
4FC95EFE4

Note that this is longer than an unsigned int (which is 4 bytes) can hold.  So shift will end up being 0xFC95EFE4 and later code will fail with ENODEV.

[Comment 1 ferret 2008-09-23 21:28:23 UTC]

Created attachment 166204 [details, diff]
suspend-overflow-gentoo.patch

suspend-overflow-gentoo.patch

Changed 'unsigned int shift' to 'loff_t shift' -- the same type as resume_offset and a much more sensible value to pass as the second argument to lseek()

[Comment 2 Peter Alfredsen (RETIRED) 2008-09-24 19:40:49 UTC]

I have sent this patch upstream and am waiting for a reply. Expect my mail to arrive shortly here:
https://sourceforge.net/mailarchive/forum.php?forum_name=suspend-devel

[Comment 3 Peter Alfredsen (RETIRED) 2008-09-26 07:42:00 UTC]

Keeping this bug up to date:

Luca Tettamonti writes:
On Wed, Sep 24, 2008 at 9:39 PM, Peter Alfredsen <[e-mail]> wrote:
> [I'm not subscribed to this mailing list. Please CC me on all replies]
>
> One of our users discovered a bug in the suspend program which may cause
> unplanned program termination. I am sending it unchanged for your
> review.
>
> Gentoo bug:
> https://bugs.gentoo.org/show_bug.cgi?id=238511
>
> ferret <[e-mail]> writes:
>> When I first configured and tried s2disk, it seemed to work (splash
>> progress bar and all) up to the point when I would expect it to power
>> off the system. It then prints an S and a newline and then returns to
>> the shell.
>>
>> I tracked the problem down to this C code in suspend.c and resume.c:
>>
>> 1388:   unsigned int size = sizeof(struct swsusp_header);
>> 1389:   unsigned int shift = (resume_offset + 1) * page_size - size;
>>
>> Since I was using a swap file, my resume_offset is 528894, page_size
>> is 4096, and size is 28 (I have checked these values by inserting a
>> printf in the above code):
>>
>> $ printf "%X\n" $(( (5228894 + 1) * 4096 - 28 ))
>> 4FC95EFE4
>>
>> Note that this is longer than an unsigned int (which is 4 bytes) can
>> hold.  So shift will end up being 0xFC95EFE4 and later code will fail
>> with ENODEV.

The bug and the analisys are correct, but the fix only works for 64bit systems.

> Patch:
>
> Author: ferret <[e-mail]>
>
> diff -urNd suspend-0.8/resume.c suspend-0.8~/resume.c
> --- suspend-0.8/resume.c        2007-12-31 18:50:12.000000000 +0000
> +++ suspend-0.8~/resume.c       2008-09-23 22:20:32.000000000 +0100
> @@ -550,7 +550,7 @@
>                            struct swsusp_header *swsusp_header)
>  {
>        unsigned int size = sizeof(struct swsusp_header);
> -       unsigned int shift = (resume_offset + 1) * page_size - size;
> +       loff_t shift = (resume_offset + 1) * page_size - size;
>        int fd, ret;

lseek takes an off_t which is a long; the patch works fine on x86_64,
but on plain x86 a long is still 32bit wide so the value passed to
lseek is truncated.
I believe that the proper fix is to use lseek64.

[Comment 4 ferret 2009-03-20 01:45:26 UTC]

Created attachment 185590 [details, diff]
updated patch that should work on 32-bit and 64-bit platforms

Sorry it took me so long to come back to this.  I've done some more research and looked at the mailing list responses (see URL above) and changed the patch.  It now uses off_t.

As is mentioned in the mailing list (amongst various confusions), this type and the associated calls are coerced into 64 bits even on 32-bit platforms by autoconf, unless ./configure --disable-largefile is used.

Currently a different patch has been applied which circumvents autoconf and changes all the lseek calls to lseek64, which seems like the wrong solution (although it does work).

I leave it up to Peter to decide whether to take issue with this upstream and relay my comments and patch there. :-)

[Comment 5 Daniel Black (RETIRED) 2009-05-10 06:33:13 UTC]

thanks - added in 0.8-r1. I think you've the patch right here.

[Comment 6 Dennis Schridde 2009-05-10 11:49:57 UTC]

!!! A file listed in the Manifest could not be found: $PORTDIR/sys-power/suspend/files/suspend-overflow-gentoo.patch

[Comment 7 Rainer 'Siju' Sigl 2009-05-10 15:33:17 UTC]

Dennis, same bug here ...

Workaround: (as root)
mkdir /usr/portage/sys-power/suspend/file
curl "http://bugs.gentoo.org/attachment.cgi?id=185590&action=view" > suspend-overflow-gentoo.patch

emerge suspend


and be happy :-)

[Comment 8 Daniel Black (RETIRED) 2009-05-10 22:32:01 UTC]

just fixed now. sorry about that. was having a few troubles with the commit process and missed this file. have fun