[PATCH] suspend.c: make sure stdin, stdout and stderr are open

[PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Stefan Rompf]

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

Hi,

recently I patched my 2.6.16 kernel to use uswsusp. Nice work, but when I 
installed /usr/local/sbin/suspend into the Suse 9.3 powermanagement scripts, 
I've stumbled over a bad interaction: powersaved closes all filedescriptors 
before calling scripts. suspend then opens snapshot and swap device, 
allocating file descriptors 0 and 1, but printfs() afterwards. Luckily, I did 
not lose any data.

For robustness, suspend must make sure that these fds are open. Simple patch 
to achieve this is attached. It loses the descriptors, but I don't think 
that's an issue for the short running program.

Stefan

[-- Attachment #2: suspend.diff --]
[-- Type: text/x-diff, Size: 713 bytes --]

Index: suspend.c
===================================================================
RCS file: /cvsroot/suspend/suspend/suspend.c,v
retrieving revision 1.33
diff -u -p -r1.33 suspend.c
--- suspend.c	21 Mar 2006 20:44:31 -0000	1.33
+++ suspend.c	24 Mar 2006 18:55:52 -0000
@@ -831,6 +830,10 @@ int main(int argc, char *argv[])
 	dev_t resume_dev;
 	int orig_loglevel, ret = 0;
 
+	/* Easy way to make sure that stdin, stdout and stderr are open
+	   so that later printfs wont go to our suspend partition */
+	while((unsigned)open("/dev/null", O_RDWR)<2);
+
 	if (get_config("suspend", argc, argv, PARAM_NO, parameters, resume_dev_name))
 		return EINVAL;
 	if (compute_checksum != 'y' && compute_checksum != 'Y')

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Rafael J. Wysocki]

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

Hi,

On Friday 24 March 2006 20:13, Stefan Rompf wrote:
> recently I patched my 2.6.16 kernel to use uswsusp. Nice work, but when I 
> installed /usr/local/sbin/suspend into the Suse 9.3 powermanagement scripts, 
> I've stumbled over a bad interaction: powersaved closes all filedescriptors 
> before calling scripts. suspend then opens snapshot and swap device, 
> allocating file descriptors 0 and 1, but printfs() afterwards. Luckily, I did 
> not lose any data.
> 
> For robustness, suspend must make sure that these fds are open. Simple patch 
> to achieve this is attached. It loses the descriptors, but I don't think 
> that's an issue for the short running program.

Thanks a lot for the patch, I'll have a look at it shortly.

BTW, it follows from what you said that you compiled suspend on SuSE 9.3.
Was it on i386?

Rafael

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Rafael J. Wysocki]

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

On Friday 24 March 2006 21:36, Rafael J. Wysocki wrote:
> On Friday 24 March 2006 20:13, Stefan Rompf wrote:
> > recently I patched my 2.6.16 kernel to use uswsusp. Nice work, but when I 
> > installed /usr/local/sbin/suspend into the Suse 9.3 powermanagement scripts, 
> > I've stumbled over a bad interaction: powersaved closes all filedescriptors 
> > before calling scripts. suspend then opens snapshot and swap device, 
> > allocating file descriptors 0 and 1, but printfs() afterwards. Luckily, I did 
> > not lose any data.

Fortunately it drops the 0, 1 and 2 descriptors afterwards in prepare_console(). :-)

Still this is a bug and has to be fixed.

I've redone your patch a bit, could you please test it?

Rafael

---
 suspend.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletion(-)

Index: suspend/suspend.c
===================================================================
--- suspend.orig/suspend.c
+++ suspend/suspend.c
@@ -856,7 +856,7 @@ int main(int argc, char *argv[])
 	struct stat stat_buf;
 	int resume_fd, snapshot_fd, vt_fd, orig_vc = -1, suspend_vc = -1;
 	dev_t resume_dev;
-	int orig_loglevel, ret = 0;
+	int orig_loglevel, ret;
 
 	if (get_config("suspend", argc, argv, PARAM_NO, parameters, resume_dev_name))
 		return EINVAL;
@@ -875,6 +875,18 @@ int main(int argc, char *argv[])
 	if (s2ram != 'y' && s2ram != 'Y')
 		s2ram = 0;
 
+	/* Make sure the 0, 1, 2 descriptors are open before opening the
+	 * snapshot and resume devices
+	 */
+	do {
+		ret = open("/dev/null", O_RDWR);
+		if (ret < 0) {
+			fprintf(stderr, "suspend: Could not open /dev/null\n");
+			return errno;
+		}
+	} while (ret <= 2);
+	close(ret);
+
 	setvbuf(stdout, NULL, _IONBF, 0);
 	setvbuf(stderr, NULL, _IONBF, 0);
 
@@ -898,6 +910,7 @@ int main(int argc, char *argv[])
 	}
 	resume_dev = stat_buf.st_rdev;
 
+	ret = 0;
 	if (stat(snapshot_dev_name, &stat_buf)) {
 		fprintf(stderr, "suspend: Could not stat the snapshot device file\n");
 		ret = ENODEV;

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Stefan Rompf]

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

Am Freitag 24 März 2006 23:55 schrieb Rafael J. Wysocki:

> I've redone your patch a bit, could you please test it?

Works nicely, thanks, but why did you move the loop downwards, so that it is 
executed after reading config and calling into libcrypto?

As you asked for the distribution I use, it is SuSE 9.3 on i386, 2.6.16 with 
some patches (uswsusp, RFC2863-operstate, avoid "udev vc" bloat).

Stefan


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Rafael J. Wysocki]

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

On Saturday 25 March 2006 09:20, Stefan Rompf wrote:
> Am Freitag 24 März 2006 23:55 schrieb Rafael J. Wysocki:
> 
> > I've redone your patch a bit, could you please test it?
> 
> Works nicely, thanks, but why did you move the loop downwards, so that it is 
> executed after reading config and calling into libcrypto?

For no particular reason.  You're right it should go earlier, I'll change that.

> As you asked for the distribution I use, it is SuSE 9.3 on i386, 2.6.16 with 
> some patches (uswsusp, RFC2863-operstate, avoid "udev vc" bloat).

Thanks.  I'm having problems with compiling vbetool on x86_64 SuSE 9.3,
but it's good to know i386 is fine.

Greetings,
Rafael



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] suspend.c: make sure stdin, stdout and stderr are open

[Stefan Seyfried]

On Fri, Mar 24, 2006 at 08:13:48PM +0100, Stefan Rompf wrote:
> Hi,
> 
> recently I patched my 2.6.16 kernel to use uswsusp. Nice work, but when I 
> installed /usr/local/sbin/suspend into the Suse 9.3 powermanagement scripts, 
> I've stumbled over a bad interaction: powersaved closes all filedescriptors 
> before calling scripts. suspend then opens snapshot and swap device, 

FWIW, i will also adress this in powersaved. More programs will run into
the same trap probably ;-)
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen