>From 9a503ec7d7fa16e09abab53cc7f768b72020ce08 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Fri, 7 May 2010 13:30:42 +0100 Subject: [PATCH] daemon: Fix read-file so it fails gracefully for large files (RHBZ#589039). Pengzhen Cao noticed that read-file would fail for files larger than the protocol size; this is *not* the bug. However it would also lose protocol synchronization after this. The reason was that functions which return RBufferOut in the generator must not 'touch' the *size_r parameter along error return paths. I fixed read-file and initrd-cat, and I checked that pread was doing the right thing. This also adds regression tests for read-file with various categories of large file. --- daemon/file.c | 14 ++++++++++---- daemon/initrd.c | 12 ++++++++---- src/generator.ml | 15 ++++++++++++++- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/daemon/file.c b/daemon/file.c index 7a0f8f9..2399828 100644 --- a/daemon/file.c +++ b/daemon/file.c @@ -336,25 +336,24 @@ do_read_file (const char *path, size_t *size_r) return NULL; } - *size_r = statbuf.st_size; /* The actual limit on messages is smaller than this. This * check just limits the amount of memory we'll try and allocate * here. If the message is larger than the real limit, that will * be caught later when we try to serialize the message. */ - if (*size_r >= GUESTFS_MESSAGE_MAX) { + if (statbuf.st_size >= GUESTFS_MESSAGE_MAX) { reply_with_error ("%s: file is too large for the protocol, use guestfs_download instead", path); close (fd); return NULL; } - r = malloc (*size_r); + r = malloc (statbuf.st_size); if (r == NULL) { reply_with_perror ("malloc"); close (fd); return NULL; } - if (xread (fd, r, *size_r) == -1) { + if (xread (fd, r, statbuf.st_size) == -1) { reply_with_perror ("read: %s", path); close (fd); free (r); @@ -367,6 +366,10 @@ do_read_file (const char *path, size_t *size_r) return NULL; } + /* Mustn't touch *size_r until we are sure that we won't return any + * error (RHBZ#589039). + */ + *size_r = statbuf.st_size; return r; } @@ -418,6 +421,9 @@ do_pread (const char *path, int count, int64_t offset, size_t *size_r) return NULL; } + /* Mustn't touch *size_r until we are sure that we won't return any + * error (RHBZ#589039). + */ *size_r = r; return buf; } diff --git a/daemon/initrd.c b/daemon/initrd.c index addeb14..932564b 100644 --- a/daemon/initrd.c +++ b/daemon/initrd.c @@ -142,25 +142,24 @@ do_initrd_cat (const char *path, const char *filename, size_t *size_r) goto cleanup; } - *size_r = statbuf.st_size; /* The actual limit on messages is smaller than this. This * check just limits the amount of memory we'll try and allocate * here. If the message is larger than the real limit, that will * be caught later when we try to serialize the message. */ - if (*size_r >= GUESTFS_MESSAGE_MAX) { + if (statbuf.st_size >= GUESTFS_MESSAGE_MAX) { reply_with_error ("%s:%s: file is too large for the protocol", path, filename); goto cleanup; } - ret = malloc (*size_r); + ret = malloc (statbuf.st_size); if (ret == NULL) { reply_with_perror ("malloc"); goto cleanup; } - if (xread (fd, ret, *size_r) == -1) { + if (xread (fd, ret, statbuf.st_size) == -1) { reply_with_perror ("read: %s:%s", path, filename); free (ret); ret = NULL; @@ -175,6 +174,11 @@ do_initrd_cat (const char *path, const char *filename, size_t *size_r) } fd = -1; + /* Mustn't touch *size_r until we are sure that we won't return any + * error (RHBZ#589039). + */ + *size_r = statbuf.st_size; + cleanup: if (fd >= 0) close (fd); diff --git a/src/generator.ml b/src/generator.ml index cbed94b..9dd8f8c 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -3272,7 +3272,20 @@ for full details."); ("read_file", (RBufferOut "content", [Pathname "path"]), 150, [ProtocolLimitWarning], [InitISOFS, Always, TestOutputBuffer ( - [["read_file"; "/known-4"]], "abc\ndef\nghi")], + [["read_file"; "/known-4"]], "abc\ndef\nghi"); + (* Test various near large, large and too large files (RHBZ#589039). *) + InitBasicFS, Always, TestLastFail ( + [["touch"; "/a"]; + ["truncate_size"; "/a"; "4194303"]; (* GUESTFS_MESSAGE_MAX - 1 *) + ["read_file"; "/a"]]); + InitBasicFS, Always, TestLastFail ( + [["touch"; "/a"]; + ["truncate_size"; "/a"; "4194304"]; (* GUESTFS_MESSAGE_MAX *) + ["read_file"; "/a"]]); + InitBasicFS, Always, TestLastFail ( + [["touch"; "/a"]; + ["truncate_size"; "/a"; "41943040"]; (* GUESTFS_MESSAGE_MAX * 10 *) + ["read_file"; "/a"]])], "read a file", "\ This calls returns the contents of the file C as a -- 1.6.6.1