On 08/01/2012 03:32 PM, Jakub Filak wrote:
* the old implementation checks only presence of the time file
* now we check validity of the time file
Signed-off-by: Jakub Filak <jfilak(a)redhat.com>
---
src/lib/dump_dir.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
index eef01f7..303954c 100644
--- a/src/lib/dump_dir.c
+++ b/src/lib/dump_dir.c
@@ -109,6 +109,58 @@ static bool exist_file_dir(const char *path)
return false;
}
+/* Returns true if the file is not readable or */
+/* if the file doesn't contain valid unixt time stamp */
+/* implementation is a compromise between efficiency and accuracy */
+/* the function gets false negative results for big numbers */
+static bool not_valid_time_file(const char *filename)
+{
+ /* Open input file, and parse it. */
+ int fd = open(filename, O_RDONLY);
+ if (fd < 0)
+ {
+ VERB2 perror_msg("Can't open '%s'", filename);
+ return true;
+ }
+
+ /* ~ maximal number of digits for positive time stamp string*/
+ char time_buf[sizeof(time_t) * 3 + 1];
+ const ssize_t rdsz = read(fd, time_buf, sizeof(time_buf));
+
+ /* Just reading, so no need to check the returned value. */
+ close(fd);
+
+ if (rdsz == -1)
Out of paranoia, I usually do (rdsz < 0) check instead.
+ {
+ VERB2 perror_msg("Can't read from '%s'", filename);
+ return true;
+ }
+
+ /* approximate maximal number of digits in timestamp is sizeof(time_t)*3 */
+ /* buffer has this size + 1 byte for trailing '\0' */
+ /* if whole size of buffer was read then file is bigger */
+ /* than string representing maximal time stamp */
+ if (rdsz == sizeof(time_buf))
+ {
+ VERB2 log("File '%s' is too long to be valid unix "
+ "time stamp (max size %zdB)", filename, sizeof(time_buf));
+ return true;
+ }
+
+ time_buf[rdsz] = '\0';
I would do this instead:
/* Our tools don't put trailing newline into time file,
* but we allow such format too:
*/
if (rdsz > 0 && time_buf[rdsz - 1] == '\n')
rdsz--;
time_buf[rdsz] = '\0';
+
+ /* range should be OK because of file size condition above */
What does this comment mean? "range"?
+ /* hence check if the string consists only from digits */
+ if (!isdigit_str(time_buf))
+ {
+ VERB2 log("File '%s' doesn't contain valid unix "
+ "time stamp ('%s')", filename, time_buf);
+ return true;
+ }
+
+ return false;
+}
+
/* Return values:
* -1: error (in this case, errno is 0 if error message is already logged)
* 0: failed to lock (someone else has it locked)
@@ -203,7 +255,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int
flags)
if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */
{
strcpy(lock_buf + dirname_len, "/"FILENAME_TIME);
- if (access(lock_buf, F_OK) != 0)
+ if (not_valid_time_file(lock_buf))
{
/* time file doesn't exist. We managed to lock the directory
* which was just created by somebody else, or is almost deleted
@@ -212,7 +264,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int
flags)
*/
strcpy(lock_buf + dirname_len, "/.lock");
xunlink(lock_buf);
- VERB1 log("Unlocked '%s' (no time file)", lock_buf);
+ VERB1 log("Unlocked '%s' (no or corrupted time file)",
lock_buf);
if (--count == 0)
{
errno = EISDIR; /* "this is an ordinary dir, not dump dir" */
@@ -303,7 +355,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
&& access(dir, R_OK) == 0
) {
char *time_file_name = concat_path_file(dir, FILENAME_TIME);
- if (access(time_file_name, R_OK) != 0)
+ if (not_valid_time_file(time_file_name))
{
dd_close(dd);
dd = NULL;
--
vda