libsemanage/src/semanage_store.c:semanage_split_fc

libsemanage/src/semanage_store.c:semanage_split_fc

[Serge E. Hallyn]

In semanage_split_fc(), linex 887 and 893, the file_con fd is not closed
on error.  Trivial patch follows.

-serge

Index: libsemanage/src/semanage_store.c
===================================================================
--- libsemanage.orig/src/semanage_store.c	2006-02-15 10:47:23.000000000 -0600
+++ libsemanage/src/semanage_store.c	2006-03-06 16:14:48.000000000 -0600
@@ -885,11 +885,13 @@ int semanage_split_fc(semanage_handle_t 
 	fc = open(semanage_path(SEMANAGE_TMP, SEMANAGE_FC), O_WRONLY | O_CREAT | O_TRUNC,  S_IRUSR | S_IWUSR);
 	if (!fc) {
 		ERR(sh, "Could not open %s for writing.", semanage_path(SEMANAGE_TMP, SEMANAGE_FC));
+		fclose(file_con);
 		return -1;
 	}
 	hd = open(semanage_path(SEMANAGE_TMP, SEMANAGE_HOMEDIR_TMPL), O_WRONLY | O_CREAT | O_TRUNC,  S_IRUSR | S_IWUSR);
 	if (!hd) {
 		ERR(sh, "Could not open %s for writing.", semanage_path(SEMANAGE_TMP, SEMANAGE_HOMEDIR_TMPL));
+		fclose(file_con);
 		close(fc);
 		return -1;
 	}

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsemanage/src/semanage_store.c:semanage_split_fc

[Ivan Gyurdiev]

Serge E. Hallyn wrote:
> In semanage_split_fc(), linex 887 and 893, the file_con fd is not closed
> on error.  Trivial patch follows.
>
>   
There's already a cleanup: goto target, I think that should be used instead
(after initializing the file descriptors, and making the close 
statements conditional).

It'd be nice if close(-1) and fclose(NULL) were no-ops.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsemanage/src/semanage_store.c:semanage_split_fc

[Stephen Smalley]

On Mon, 2006-03-06 at 21:03 -0500, Ivan Gyurdiev wrote:
> Serge E. Hallyn wrote:
> > In semanage_split_fc(), linex 887 and 893, the file_con fd is not closed
> > on error.  Trivial patch follows.
> >
> >   
> There's already a cleanup: goto target, I think that should be used instead
> (after initializing the file descriptors, and making the close 
> statements conditional).
> 
> It'd be nice if close(-1) and fclose(NULL) were no-ops.

Like this?

Index: libsemanage/src/semanage_store.c
===================================================================
RCS file: /nfshome/pal/CVS/selinux-usr/libsemanage/src/semanage_store.c,v
retrieving revision 1.43
diff -u -p -r1.43 semanage_store.c
--- libsemanage/src/semanage_store.c	15 Feb 2006 16:23:45 -0000	1.43
+++ libsemanage/src/semanage_store.c	8 Mar 2006 14:46:51 -0000
@@ -871,27 +871,26 @@ hidden_def(semanage_reload_policy)
 
 /* This expands the file_context.tmpl file to file_context and homedirs.template */
 int semanage_split_fc(semanage_handle_t *sh) {
-	FILE *file_con;
-	int fc, hd, retval = 0;
+	FILE *file_con = NULL;
+	int fc = -1, hd = -1, retval = -1;
 	char buf[PATH_MAX] = {0};
 
 	/* I use fopen here instead of open so that I can use fgets which only reads a single line */
 	file_con = fopen(semanage_path(SEMANAGE_TMP, SEMANAGE_FC_TMPL), "r");
 	if (!file_con) {
 		ERR(sh, "Could not open %s for reading.", semanage_path(SEMANAGE_TMP, SEMANAGE_FC_TMPL));
-		return -1;
+		goto cleanup;
 	}
 
 	fc = open(semanage_path(SEMANAGE_TMP, SEMANAGE_FC), O_WRONLY | O_CREAT | O_TRUNC,  S_IRUSR | S_IWUSR);
 	if (!fc) {
 		ERR(sh, "Could not open %s for writing.", semanage_path(SEMANAGE_TMP, SEMANAGE_FC));
-		return -1;
+		goto cleanup;
 	}
 	hd = open(semanage_path(SEMANAGE_TMP, SEMANAGE_HOMEDIR_TMPL), O_WRONLY | O_CREAT | O_TRUNC,  S_IRUSR | S_IWUSR);
 	if (!hd) {
 		ERR(sh, "Could not open %s for writing.", semanage_path(SEMANAGE_TMP, SEMANAGE_HOMEDIR_TMPL));
-		close(fc);
-		return -1;
+		goto cleanup;
 	}
 
 	while (fgets_unlocked(buf, PATH_MAX, file_con)) {
@@ -901,22 +900,24 @@ int semanage_split_fc(semanage_handle_t 
 			/* This contains one of the template variables, write it to homedir.template */
 			if (write(hd, buf, strnlen(buf, PATH_MAX)) == 0) {
 				ERR(sh, "Write to %s failed.", semanage_path(SEMANAGE_TMP, SEMANAGE_HOMEDIR_TMPL));
-				retval = -1;
 				goto cleanup;
 			}
 		} else {
 			if (write(fc, buf, strnlen(buf, PATH_MAX)) == 0) {
 				ERR(sh, "Write to %s failed.", semanage_path(SEMANAGE_TMP, SEMANAGE_FC));
-				retval = -1;	
 				goto cleanup;
 			}
 		}
 	}
-	
+
+	retval = 0;
 cleanup:
-	fclose(file_con);
-	close(fc);
-	close(hd);	
+	if (file_con)
+		fclose(file_con);
+	if (fc >= 0)
+		close(fc);
+	if (hd >= 0)
+		close(hd);
 
 	return retval;
 		

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: libsemanage/src/semanage_store.c:semanage_split_fc

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Mon, 2006-03-06 at 21:03 -0500, Ivan Gyurdiev wrote:
> > Serge E. Hallyn wrote:
> > > In semanage_split_fc(), linex 887 and 893, the file_con fd is not closed
> > > on error.  Trivial patch follows.
> > >
> > >   
> > There's already a cleanup: goto target, I think that should be used instead
> > (after initializing the file descriptors, and making the close 
> > statements conditional).
> > 
> > It'd be nice if close(-1) and fclose(NULL) were no-ops.
> 
> Like this?

Agreed, looks far more maintainable.

thanks,
-serge

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.