diff mbox

[5/5] lib: fwts_olog: avoid TOCTOU race on stat and fopen

Message ID 1459619524-12635-6-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King April 2, 2016, 5:52 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

stat'ing a file and then opening it is always a potential race condition
as a user can change the file between the stat and the open. Only
stat the file after it is open and also on the open check for the
errno on a failed open and handle the file does not exist special
case like the original stat did.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_olog.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Alex Hung April 6, 2016, 2:44 a.m. UTC | #1 
On 2016-04-03 01:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> stat'ing a file and then opening it is always a potential race condition
> as a user can change the file between the stat and the open. Only
> stat the file after it is open and also on the open check for the
> errno on a failed open and handle the file does not exist special
> case like the original stat did.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_olog.c | 35 +++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/src/lib/src/fwts_olog.c b/src/lib/src/fwts_olog.c
> index 132efc4..12d693c 100644
> --- a/src/lib/src/fwts_olog.c
> +++ b/src/lib/src/fwts_olog.c
> @@ -28,6 +28,7 @@
>   #include <sys/stat.h>
>   #include <regex.h>
>   #include <fcntl.h>
> +#include <errno.h>
>
>   #include "fwts.h"
>
> @@ -57,20 +58,6 @@ fwts_list *fwts_olog_read(fwts_framework *fw)
>   	FILE* msglog_outfile_f;
>
>   	/*
> -	 * Check for the existance of the opal msglog and only if it exists
> -	 * dump it out.  This makes the use of the OLOG as a custom option
> -	 * and not just for PPC.  We don't use compiler flags since we want
> -	 * to run this as a custom job cross platform
> -	 */
> -	if (stat(msglog, &filestat)) {
> -		/*
> -		 * stat fails so not PPC with OPAL msglog and
> -		 * no -o OLOG sent
> -		 */
> -		return NULL;
> -	}
> -
> -	/*
>   	 * Special file handling to sequentially fread the sysfs msglog into
>   	 * a static buffer based on inodes in the stat.  The sysfs msglog has
>   	 * a 0 byte file size since it is a sysfs object.
> @@ -80,17 +67,33 @@ fwts_list *fwts_olog_read(fwts_framework *fw)
>   	 */
>   	if (!(msglog_f = fopen(msglog, "r"))) {
>   		/* open the sysfs msglog for read only */
> +		if (errno == ENOENT) {
> +			/*
> +			 * If file does not exist, treat as non-fatal
> +			 * for non PPC devices that don't have the
> +			 * arch specific sys file.
> +			 */
> +			return NULL;
> +		}
>   		goto olog_common_exit;
>   	}
>
> +	if (fstat(fileno(msglog_f), &filestat)) {
> +		/*
> +		 * stat fails so not PPC with OPAL msglog and
> +		 * no -o OLOG sent
> +		 */
> +		fclose(msglog_f);
> +		return NULL;
> +	}
> +
>   	if ((len = filestat.st_blksize) < 1) {
>   		/* if any size at all continue */
>   		goto olog_cleanup_msglog;
>   	}
>
> -	if ((buffer = calloc(1, len + 1)) == NULL) {
> +	if ((buffer = calloc(1, len + 1)) == NULL)
>   		goto olog_cleanup_msglog;
> -	}
>
>   	if (!(msglog_outfile_f = fopen(msglog_outfile, "w"))) {
>   		/* create the output file for the sysfs msglog */
>


Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu April 7, 2016, 7:39 a.m. UTC | #2 
On 2016年04月03日 01:52, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> stat'ing a file and then opening it is always a potential race condition
> as a user can change the file between the stat and the open. Only
> stat the file after it is open and also on the open check for the
> errno on a failed open and handle the file does not exist special
> case like the original stat did.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_olog.c | 35 +++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/src/lib/src/fwts_olog.c b/src/lib/src/fwts_olog.c
> index 132efc4..12d693c 100644
> --- a/src/lib/src/fwts_olog.c
> +++ b/src/lib/src/fwts_olog.c
> @@ -28,6 +28,7 @@
>   #include <sys/stat.h>
>   #include <regex.h>
>   #include <fcntl.h>
> +#include <errno.h>
>
>   #include "fwts.h"
>
> @@ -57,20 +58,6 @@ fwts_list *fwts_olog_read(fwts_framework *fw)
>   	FILE* msglog_outfile_f;
>
>   	/*
> -	 * Check for the existance of the opal msglog and only if it exists
> -	 * dump it out.  This makes the use of the OLOG as a custom option
> -	 * and not just for PPC.  We don't use compiler flags since we want
> -	 * to run this as a custom job cross platform
> -	 */
> -	if (stat(msglog, &filestat)) {
> -		/*
> -		 * stat fails so not PPC with OPAL msglog and
> -		 * no -o OLOG sent
> -		 */
> -		return NULL;
> -	}
> -
> -	/*
>   	 * Special file handling to sequentially fread the sysfs msglog into
>   	 * a static buffer based on inodes in the stat.  The sysfs msglog has
>   	 * a 0 byte file size since it is a sysfs object.
> @@ -80,17 +67,33 @@ fwts_list *fwts_olog_read(fwts_framework *fw)
>   	 */
>   	if (!(msglog_f = fopen(msglog, "r"))) {
>   		/* open the sysfs msglog for read only */
> +		if (errno == ENOENT) {
> +			/*
> +			 * If file does not exist, treat as non-fatal
> +			 * for non PPC devices that don't have the
> +			 * arch specific sys file.
> +			 */
> +			return NULL;
> +		}
>   		goto olog_common_exit;
>   	}
>
> +	if (fstat(fileno(msglog_f), &filestat)) {
> +		/*
> +		 * stat fails so not PPC with OPAL msglog and
> +		 * no -o OLOG sent
> +		 */
> +		fclose(msglog_f);
> +		return NULL;
> +	}
> +
>   	if ((len = filestat.st_blksize) < 1) {
>   		/* if any size at all continue */
>   		goto olog_cleanup_msglog;
>   	}
>
> -	if ((buffer = calloc(1, len + 1)) == NULL) {
> +	if ((buffer = calloc(1, len + 1)) == NULL)
>   		goto olog_cleanup_msglog;
> -	}
>
>   	if (!(msglog_outfile_f = fopen(msglog_outfile, "w"))) {
>   		/* create the output file for the sysfs msglog */
>

Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_olog.c b/src/lib/src/fwts_olog.c
index 132efc4..12d693c 100644
--- a/src/lib/src/fwts_olog.c
+++ b/src/lib/src/fwts_olog.c
@@ -28,6 +28,7 @@ 
 #include <sys/stat.h>
 #include <regex.h>
 #include <fcntl.h>
+#include <errno.h>
 
 #include "fwts.h"
 
@@ -57,20 +58,6 @@  fwts_list *fwts_olog_read(fwts_framework *fw)
 	FILE* msglog_outfile_f;
 
 	/*
-	 * Check for the existance of the opal msglog and only if it exists
-	 * dump it out.  This makes the use of the OLOG as a custom option
-	 * and not just for PPC.  We don't use compiler flags since we want
-	 * to run this as a custom job cross platform
-	 */
-	if (stat(msglog, &filestat)) {
-		/*
-		 * stat fails so not PPC with OPAL msglog and
-		 * no -o OLOG sent
-		 */
-		return NULL;
-	}
-
-	/*
 	 * Special file handling to sequentially fread the sysfs msglog into
 	 * a static buffer based on inodes in the stat.  The sysfs msglog has
 	 * a 0 byte file size since it is a sysfs object.
@@ -80,17 +67,33 @@  fwts_list *fwts_olog_read(fwts_framework *fw)
 	 */
 	if (!(msglog_f = fopen(msglog, "r"))) {
 		/* open the sysfs msglog for read only */
+		if (errno == ENOENT) {
+			/*
+			 * If file does not exist, treat as non-fatal
+			 * for non PPC devices that don't have the
+			 * arch specific sys file.
+			 */
+			return NULL;
+		}
 		goto olog_common_exit;
 	}
 
+	if (fstat(fileno(msglog_f), &filestat)) {
+		/*
+		 * stat fails so not PPC with OPAL msglog and
+		 * no -o OLOG sent
+		 */
+		fclose(msglog_f);
+		return NULL;
+	}
+
 	if ((len = filestat.st_blksize) < 1) {
 		/* if any size at all continue */
 		goto olog_cleanup_msglog;
 	}
 
-	if ((buffer = calloc(1, len + 1)) == NULL) {
+	if ((buffer = calloc(1, len + 1)) == NULL)
 		goto olog_cleanup_msglog;
-	}
 
 	if (!(msglog_outfile_f = fopen(msglog_outfile, "w"))) {
 		/* create the output file for the sysfs msglog */