* [PATCH] fix memory leaks and uninitialized jump
@ 2015-10-26 18:32 william.c.roberts
2015-10-26 18:42 ` Roberts, William C
2015-10-27 17:40 ` Stephen Smalley
0 siblings, 2 replies; 6+ messages in thread
From: william.c.roberts @ 2015-10-26 18:32 UTC (permalink / raw
To: selinux; +Cc: sds, William Roberts
From: William Roberts <william.c.roberts@intel.com>
Some error's were reported by valgrind (below) fix them. The test
cases on which these leaks were detected:
1. properly formed file_contexts file.
2. malformed file_contexts file, unknown type.
3. malformed file_contexts file, type that fails on validate callback.
4. malformed file_contexts file, invalid regex.
5. malformed file_contexts file, invalid mode.
==3819== Conditional jump or move depends on uninitialised value(s)
==3819== at 0x12A682: closef (label_file.c:577)
==3819== by 0x12A196: selabel_close (label.c:163)
==3819== by 0x10A2FD: cleanup (checkfc.c:218)
==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
==3819== by 0x50892A4: exit (exit.c:104)
==3819== by 0x10A231: main (checkfc.c:361)
==3819== Uninitialised value was created by a heap allocation
==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3819== by 0x4C2CF1F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3819== by 0x12BB31: process_file (label_file.h:273)
==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
==3819== by 0x12A0BB: selabel_open (label.c:88)
==3819== by 0x10A038: main (checkfc.c:292)
==3819==
==3819==
==3819== HEAP SUMMARY:
==3819== in use at exit: 729 bytes in 19 blocks
==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854 bytes allocated
==3819==
==3819== 81 bytes in 1 blocks are definitely lost in loss record 1 of 2
==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3819== by 0x50D5839: strdup (strdup.c:42)
==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
==3819== by 0x12A0BB: selabel_open (label.c:88)
==3819== by 0x10A038: main (checkfc.c:292)
==3819==
==4238== 40 bytes in 1 blocks are definitely lost in loss record 1 of 6
==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
==4238== by 0x12A0BB: selabel_open (label.c:88)
==4238== by 0x10A038: main (checkfc.c:292)
==4238==
==4238== 81 bytes in 1 blocks are definitely lost in loss record 2 of 6
==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x50D5839: strdup (strdup.c:42)
==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
==4238== by 0x12A0BB: selabel_open (label.c:88)
==4238== by 0x10A038: main (checkfc.c:292)
==4238==
==4238== 386 bytes in 24 blocks are definitely lost in loss record 3 of 6
==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x50D5889: strndup (strndup.c:45)
==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
==4238== by 0x12B72D: process_file (label_file.h:392)
==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
==4238== by 0x12A0BB: selabel_open (label.c:88)
==4238== by 0x10A038: main (checkfc.c:292)
==4238==
==4238== 648 bytes in 18 blocks are definitely lost in loss record 4 of 6
==4238== at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
==4238== by 0x117C10: avtab_insert (avtab.c:163)
==4238== by 0x11880A: avtab_read_item (avtab.c:566)
==4238== by 0x118BD3: avtab_read (avtab.c:600)
==4238== by 0x125BDD: policydb_read (policydb.c:3854)
==4238== by 0x109F87: main (checkfc.c:273)
==4238==
==4238== 1,095 bytes in 12 blocks are definitely lost in loss record 5 of 6
==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
==4238== by 0x12B239: compile_regex (label_file.h:357)
==4238== by 0x12B9C7: process_file (label_file.h:429)
==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
==4238== by 0x12A0BB: selabel_open (label.c:88)
==4238== by 0x10A038: main (checkfc.c:292)
==4238==
==4238== 1,296 bytes in 12 blocks are definitely lost in loss record 6 of 6
==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
==4238== by 0x12B25D: compile_regex (label_file.h:366)
==4238== by 0x12B9C7: process_file (label_file.h:429)
==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
==4238== by 0x12A0BB: selabel_open (label.c:88)
==4238== by 0x10A038: main (checkfc.c:292)
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
libselinux/src/label.c | 1 +
libselinux/src/label_file.c | 5 ++++-
libselinux/src/label_file.h | 22 ++++++++++++++--------
3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index c656fda..963bfcb 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int backend,
rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
if ((*initfuncs[backend])(rec, opts, nopts)) {
+ free(rec->spec_file);
free(rec);
rec = NULL;
}
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 1a0c15f..071d902 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -507,6 +507,8 @@ out:
return rc;
}
+static void closef(struct selabel_handle *rec);
+
static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
unsigned n)
{
@@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
finish:
if (status)
- free(data->spec_arr);
+ closef(rec);
+
return status;
}
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 2492826..a85b9bd 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
}
data->stem_arr[num].len = stem_len;
data->stem_arr[num].buf = buf;
+ data->stem_arr[num].from_mmap = 0;
data->num_stems++;
return num;
@@ -425,6 +426,19 @@ static inline int process_line(struct selabel_handle *rec,
/* process and store the specification in spec. */
spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
spec_arr[nspec].regex_str = regex;
+
+ /* Convert the type string to a mode format */
+ spec_arr[nspec].type_str = type;
+ spec_arr[nspec].mode = 0;
+
+ spec_arr[nspec].lr.ctx_raw = context;
+
+ /*
+ * bump data->nspecs to cause closef() to cover it in its free
+ * but do not bump nspec since it's used below.
+ */
+ data->nspec++;
+
if (rec->validating &&
compile_regex(data, &spec_arr[nspec], &errbuf)) {
COMPAT_LOG(SELINUX_ERROR,
@@ -435,9 +449,6 @@ static inline int process_line(struct selabel_handle *rec,
return -1;
}
- /* Convert the type string to a mode format */
- spec_arr[nspec].type_str = type;
- spec_arr[nspec].mode = 0;
if (type) {
mode_t mode = string_to_mode(type);
@@ -445,14 +456,11 @@ static inline int process_line(struct selabel_handle *rec,
COMPAT_LOG(SELINUX_ERROR,
"%s: line %u has invalid file type %s\n",
path, lineno, type);
- errno = EINVAL;
return -1;
}
spec_arr[nspec].mode = mode;
}
- spec_arr[nspec].lr.ctx_raw = context;
-
/* Determine if specification has
* any meta characters in the RE */
spec_hasMetaChars(&spec_arr[nspec]);
@@ -460,8 +468,6 @@ static inline int process_line(struct selabel_handle *rec,
if (strcmp(context, "<<none>>") && rec->validating)
compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
- data->nspec = ++nspec;
-
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] fix memory leaks and uninitialized jump
2015-10-26 18:32 [PATCH] fix memory leaks and uninitialized jump william.c.roberts
@ 2015-10-26 18:42 ` Roberts, William C
2015-10-27 17:48 ` Stephen Smalley
2015-10-27 17:40 ` Stephen Smalley
1 sibling, 1 reply; 6+ messages in thread
From: Roberts, William C @ 2015-10-26 18:42 UTC (permalink / raw
To: selinux@tycho.nsa.gov; +Cc: sds@tycho.nsa.gov
Shouldn't;
compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
in process_line() cause a failure? Right now the return code is being ignored.
Bill
> -----Original Message-----
> From: Roberts, William C
> Sent: Monday, October 26, 2015 11:33 AM
> To: selinux@tycho.nsa.gov
> Cc: sds@tycho.nsa.gov; Roberts, William C
> Subject: [PATCH] fix memory leaks and uninitialized jump
>
> From: William Roberts <william.c.roberts@intel.com>
>
> Some error's were reported by valgrind (below) fix them. The test
> cases on which these leaks were detected:
>
> 1. properly formed file_contexts file.
> 2. malformed file_contexts file, unknown type.
> 3. malformed file_contexts file, type that fails on validate callback.
> 4. malformed file_contexts file, invalid regex.
> 5. malformed file_contexts file, invalid mode.
>
> ==3819== Conditional jump or move depends on uninitialised value(s)
> ==3819== at 0x12A682: closef (label_file.c:577)
> ==3819== by 0x12A196: selabel_close (label.c:163)
> ==3819== by 0x10A2FD: cleanup (checkfc.c:218)
> ==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
> ==3819== by 0x50892A4: exit (exit.c:104)
> ==3819== by 0x10A231: main (checkfc.c:361)
> ==3819== Uninitialised value was created by a heap allocation
> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==3819== by 0x4C2CF1F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==3819== by 0x12BB31: process_file (label_file.h:273)
> ==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==3819== by 0x12A0BB: selabel_open (label.c:88)
> ==3819== by 0x10A038: main (checkfc.c:292)
> ==3819==
> ==3819==
> ==3819== HEAP SUMMARY:
> ==3819== in use at exit: 729 bytes in 19 blocks
> ==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854 bytes allocated
> ==3819==
> ==3819== 81 bytes in 1 blocks are definitely lost in loss record 1 of 2
> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==3819== by 0x50D5839: strdup (strdup.c:42)
> ==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
> ==3819== by 0x12A0BB: selabel_open (label.c:88)
> ==3819== by 0x10A038: main (checkfc.c:292)
> ==3819==
>
> ==4238== 40 bytes in 1 blocks are definitely lost in loss record 1 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 81 bytes in 1 blocks are definitely lost in loss record 2 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x50D5839: strdup (strdup.c:42)
> ==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 386 bytes in 24 blocks are definitely lost in loss record 3 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x50D5889: strndup (strndup.c:45)
> ==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
> ==4238== by 0x12B72D: process_file (label_file.h:392)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 648 bytes in 18 blocks are definitely lost in loss record 4 of 6
> ==4238== at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
> ==4238== by 0x117C10: avtab_insert (avtab.c:163)
> ==4238== by 0x11880A: avtab_read_item (avtab.c:566)
> ==4238== by 0x118BD3: avtab_read (avtab.c:600)
> ==4238== by 0x125BDD: policydb_read (policydb.c:3854)
> ==4238== by 0x109F87: main (checkfc.c:273)
> ==4238==
> ==4238== 1,095 bytes in 12 blocks are definitely lost in loss record 5 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
> ==4238== by 0x12B239: compile_regex (label_file.h:357)
> ==4238== by 0x12B9C7: process_file (label_file.h:429)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 1,296 bytes in 12 blocks are definitely lost in loss record 6 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> amd64-linux.so)
> ==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
> ==4238== by 0x12B25D: compile_regex (label_file.h:366)
> ==4238== by 0x12B9C7: process_file (label_file.h:429)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
> libselinux/src/label.c | 1 +
> libselinux/src/label_file.c | 5 ++++-
> libselinux/src/label_file.h | 22 ++++++++++++++--------
> 3 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index c656fda..963bfcb
> 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int
> backend,
> rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
>
> if ((*initfuncs[backend])(rec, opts, nopts)) {
> + free(rec->spec_file);
> free(rec);
> rec = NULL;
> }
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index
> 1a0c15f..071d902 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -507,6 +507,8 @@ out:
> return rc;
> }
>
> +static void closef(struct selabel_handle *rec);
> +
> static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
> unsigned n)
> {
> @@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const struct
> selinux_opt *opts,
>
> finish:
> if (status)
> - free(data->spec_arr);
> + closef(rec);
> +
> return status;
> }
>
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h index
> 2492826..a85b9bd 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data *data, char
> *buf, int stem_len)
> }
> data->stem_arr[num].len = stem_len;
> data->stem_arr[num].buf = buf;
> + data->stem_arr[num].from_mmap = 0;
> data->num_stems++;
>
> return num;
> @@ -425,6 +426,19 @@ static inline int process_line(struct selabel_handle *rec,
> /* process and store the specification in spec. */
> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> spec_arr[nspec].regex_str = regex;
> +
> + /* Convert the type string to a mode format */
> + spec_arr[nspec].type_str = type;
> + spec_arr[nspec].mode = 0;
> +
> + spec_arr[nspec].lr.ctx_raw = context;
> +
> + /*
> + * bump data->nspecs to cause closef() to cover it in its free
> + * but do not bump nspec since it's used below.
> + */
> + data->nspec++;
> +
> if (rec->validating &&
> compile_regex(data, &spec_arr[nspec], &errbuf)) {
> COMPAT_LOG(SELINUX_ERROR,
> @@ -435,9 +449,6 @@ static inline int process_line(struct selabel_handle *rec,
> return -1;
> }
>
> - /* Convert the type string to a mode format */
> - spec_arr[nspec].type_str = type;
> - spec_arr[nspec].mode = 0;
> if (type) {
> mode_t mode = string_to_mode(type);
>
> @@ -445,14 +456,11 @@ static inline int process_line(struct selabel_handle *rec,
> COMPAT_LOG(SELINUX_ERROR,
> "%s: line %u has invalid file type %s\n",
> path, lineno, type);
> - errno = EINVAL;
> return -1;
> }
> spec_arr[nspec].mode = mode;
> }
>
> - spec_arr[nspec].lr.ctx_raw = context;
> -
> /* Determine if specification has
> * any meta characters in the RE */
> spec_hasMetaChars(&spec_arr[nspec]);
> @@ -460,8 +468,6 @@ static inline int process_line(struct selabel_handle *rec,
> if (strcmp(context, "<<none>>") && rec->validating)
> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>
> - data->nspec = ++nspec;
> -
> return 0;
> }
>
> --
> 1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix memory leaks and uninitialized jump
2015-10-26 18:32 [PATCH] fix memory leaks and uninitialized jump william.c.roberts
2015-10-26 18:42 ` Roberts, William C
@ 2015-10-27 17:40 ` Stephen Smalley
2015-10-27 18:05 ` William Roberts
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2015-10-27 17:40 UTC (permalink / raw
To: william.c.roberts, selinux
Subject: [PATCH] libselinux: label_file: fix memory leaks and
uninitialized jump
On 10/26/2015 02:32 PM, william.c.roberts@intel.com wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Some error's were reported by valgrind (below) fix them. The test
> cases on which these leaks were detected:
Why do you have leading space in the subject line and every line of the
description?
>
> 1. properly formed file_contexts file.
> 2. malformed file_contexts file, unknown type.
> 3. malformed file_contexts file, type that fails on validate callback.
> 4. malformed file_contexts file, invalid regex.
> 5. malformed file_contexts file, invalid mode.
>
> ==3819== Conditional jump or move depends on uninitialised value(s)
> ==3819== at 0x12A682: closef (label_file.c:577)
> ==3819== by 0x12A196: selabel_close (label.c:163)
> ==3819== by 0x10A2FD: cleanup (checkfc.c:218)
> ==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
> ==3819== by 0x50892A4: exit (exit.c:104)
> ==3819== by 0x10A231: main (checkfc.c:361)
> ==3819== Uninitialised value was created by a heap allocation
> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3819== by 0x4C2CF1F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3819== by 0x12BB31: process_file (label_file.h:273)
> ==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==3819== by 0x12A0BB: selabel_open (label.c:88)
> ==3819== by 0x10A038: main (checkfc.c:292)
> ==3819==
> ==3819==
> ==3819== HEAP SUMMARY:
> ==3819== in use at exit: 729 bytes in 19 blocks
> ==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854 bytes allocated
> ==3819==
> ==3819== 81 bytes in 1 blocks are definitely lost in loss record 1 of 2
> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3819== by 0x50D5839: strdup (strdup.c:42)
> ==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
> ==3819== by 0x12A0BB: selabel_open (label.c:88)
> ==3819== by 0x10A038: main (checkfc.c:292)
> ==3819==
>
> ==4238== 40 bytes in 1 blocks are definitely lost in loss record 1 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 81 bytes in 1 blocks are definitely lost in loss record 2 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x50D5839: strdup (strdup.c:42)
> ==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 386 bytes in 24 blocks are definitely lost in loss record 3 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x50D5889: strndup (strndup.c:45)
> ==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
> ==4238== by 0x12B72D: process_file (label_file.h:392)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 648 bytes in 18 blocks are definitely lost in loss record 4 of 6
> ==4238== at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
> ==4238== by 0x117C10: avtab_insert (avtab.c:163)
> ==4238== by 0x11880A: avtab_read_item (avtab.c:566)
> ==4238== by 0x118BD3: avtab_read (avtab.c:600)
> ==4238== by 0x125BDD: policydb_read (policydb.c:3854)
> ==4238== by 0x109F87: main (checkfc.c:273)
> ==4238==
> ==4238== 1,095 bytes in 12 blocks are definitely lost in loss record 5 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
> ==4238== by 0x12B239: compile_regex (label_file.h:357)
> ==4238== by 0x12B9C7: process_file (label_file.h:429)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
> ==4238==
> ==4238== 1,296 bytes in 12 blocks are definitely lost in loss record 6 of 6
> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
> ==4238== by 0x12B25D: compile_regex (label_file.h:366)
> ==4238== by 0x12B9C7: process_file (label_file.h:429)
> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
> ==4238== by 0x12A0BB: selabel_open (label.c:88)
> ==4238== by 0x10A038: main (checkfc.c:292)
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
> libselinux/src/label.c | 1 +
> libselinux/src/label_file.c | 5 ++++-
> libselinux/src/label_file.h | 22 ++++++++++++++--------
> 3 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> index c656fda..963bfcb 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int backend,
> rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
>
> if ((*initfuncs[backend])(rec, opts, nopts)) {
> + free(rec->spec_file);
> free(rec);
> rec = NULL;
> }
> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
> index 1a0c15f..071d902 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -507,6 +507,8 @@ out:
> return rc;
> }
>
> +static void closef(struct selabel_handle *rec);
> +
> static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
> unsigned n)
> {
> @@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
>
> finish:
> if (status)
> - free(data->spec_arr);
> + closef(rec);
> +
> return status;
> }
>
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index 2492826..a85b9bd 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
> }
> data->stem_arr[num].len = stem_len;
> data->stem_arr[num].buf = buf;
> + data->stem_arr[num].from_mmap = 0;
> data->num_stems++;
>
> return num;
> @@ -425,6 +426,19 @@ static inline int process_line(struct selabel_handle *rec,
> /* process and store the specification in spec. */
> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> spec_arr[nspec].regex_str = regex;
> +
> + /* Convert the type string to a mode format */
Technically we should leave this comment line down where it was, at the
point that we actually convert the type string to a mode.
> + spec_arr[nspec].type_str = type;
> + spec_arr[nspec].mode = 0;
> +
> + spec_arr[nspec].lr.ctx_raw = context;
> +
> + /*
> + * bump data->nspecs to cause closef() to cover it in its free
> + * but do not bump nspec since it's used below.
> + */
> + data->nspec++;
> +
> if (rec->validating &&
> compile_regex(data, &spec_arr[nspec], &errbuf)) {
> COMPAT_LOG(SELINUX_ERROR,
> @@ -435,9 +449,6 @@ static inline int process_line(struct selabel_handle *rec,
> return -1;
> }
>
> - /* Convert the type string to a mode format */
> - spec_arr[nspec].type_str = type;
> - spec_arr[nspec].mode = 0;
> if (type) {
> mode_t mode = string_to_mode(type);
>
> @@ -445,14 +456,11 @@ static inline int process_line(struct selabel_handle *rec,
> COMPAT_LOG(SELINUX_ERROR,
> "%s: line %u has invalid file type %s\n",
> path, lineno, type);
> - errno = EINVAL;
Don't want to remove this.
> return -1;
> }
> spec_arr[nspec].mode = mode;
> }
>
> - spec_arr[nspec].lr.ctx_raw = context;
> -
> /* Determine if specification has
> * any meta characters in the RE */
> spec_hasMetaChars(&spec_arr[nspec]);
> @@ -460,8 +468,6 @@ static inline int process_line(struct selabel_handle *rec,
> if (strcmp(context, "<<none>>") && rec->validating)
> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>
> - data->nspec = ++nspec;
> -
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix memory leaks and uninitialized jump
2015-10-26 18:42 ` Roberts, William C
@ 2015-10-27 17:48 ` Stephen Smalley
2015-10-27 22:18 ` William Roberts
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2015-10-27 17:48 UTC (permalink / raw
To: Roberts, William C, selinux@tycho.nsa.gov
On 10/26/2015 02:42 PM, Roberts, William C wrote:
> Shouldn't;
> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>
> in process_line() cause a failure? Right now the return code is being ignored.
I think it is historical. Originally we had it bail on error. Red Hat
had problems with that because they would ship a file_contexts file that
had an invalid context or the user would have one bad entry in
file_contexts.local or a local module (maybe due to a policy change that
removed a type, so it might have been valid originally and then became
invalid), and suddenly restorecon/setfiles/rpm/... couldn't label
anything at all, leading to unlabeled files and possibly an unbootable
system. So then we took the error handling to the validate callback,
where setfiles or sefcontext_compile can make it a fatal error but you
can still label most files (the ones that match valid entries) when they
get installed via rpm or using restorecon.
>
> Bill
>
>> -----Original Message-----
>> From: Roberts, William C
>> Sent: Monday, October 26, 2015 11:33 AM
>> To: selinux@tycho.nsa.gov
>> Cc: sds@tycho.nsa.gov; Roberts, William C
>> Subject: [PATCH] fix memory leaks and uninitialized jump
>>
>> From: William Roberts <william.c.roberts@intel.com>
>>
>> Some error's were reported by valgrind (below) fix them. The test
>> cases on which these leaks were detected:
>>
>> 1. properly formed file_contexts file.
>> 2. malformed file_contexts file, unknown type.
>> 3. malformed file_contexts file, type that fails on validate callback.
>> 4. malformed file_contexts file, invalid regex.
>> 5. malformed file_contexts file, invalid mode.
>>
>> ==3819== Conditional jump or move depends on uninitialised value(s)
>> ==3819== at 0x12A682: closef (label_file.c:577)
>> ==3819== by 0x12A196: selabel_close (label.c:163)
>> ==3819== by 0x10A2FD: cleanup (checkfc.c:218)
>> ==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
>> ==3819== by 0x50892A4: exit (exit.c:104)
>> ==3819== by 0x10A231: main (checkfc.c:361)
>> ==3819== Uninitialised value was created by a heap allocation
>> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==3819== by 0x4C2CF1F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==3819== by 0x12BB31: process_file (label_file.h:273)
>> ==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>> ==3819== by 0x10A038: main (checkfc.c:292)
>> ==3819==
>> ==3819==
>> ==3819== HEAP SUMMARY:
>> ==3819== in use at exit: 729 bytes in 19 blocks
>> ==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854 bytes allocated
>> ==3819==
>> ==3819== 81 bytes in 1 blocks are definitely lost in loss record 1 of 2
>> ==3819== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==3819== by 0x50D5839: strdup (strdup.c:42)
>> ==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>> ==3819== by 0x10A038: main (checkfc.c:292)
>> ==3819==
>>
>> ==4238== 40 bytes in 1 blocks are definitely lost in loss record 1 of 6
>> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 81 bytes in 1 blocks are definitely lost in loss record 2 of 6
>> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x50D5839: strdup (strdup.c:42)
>> ==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 386 bytes in 24 blocks are definitely lost in loss record 3 of 6
>> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x50D5889: strndup (strndup.c:45)
>> ==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
>> ==4238== by 0x12B72D: process_file (label_file.h:392)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 648 bytes in 18 blocks are definitely lost in loss record 4 of 6
>> ==4238== at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
>> ==4238== by 0x117C10: avtab_insert (avtab.c:163)
>> ==4238== by 0x11880A: avtab_read_item (avtab.c:566)
>> ==4238== by 0x118BD3: avtab_read (avtab.c:600)
>> ==4238== by 0x125BDD: policydb_read (policydb.c:3854)
>> ==4238== by 0x109F87: main (checkfc.c:273)
>> ==4238==
>> ==4238== 1,095 bytes in 12 blocks are definitely lost in loss record 5 of 6
>> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
>> ==4238== by 0x12B239: compile_regex (label_file.h:357)
>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 1,296 bytes in 12 blocks are definitely lost in loss record 6 of 6
>> ==4238== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> amd64-linux.so)
>> ==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
>> ==4238== by 0x12B25D: compile_regex (label_file.h:366)
>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>>
>> Signed-off-by: William Roberts <william.c.roberts@intel.com>
>> ---
>> libselinux/src/label.c | 1 +
>> libselinux/src/label_file.c | 5 ++++-
>> libselinux/src/label_file.h | 22 ++++++++++++++--------
>> 3 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index c656fda..963bfcb
>> 100644
>> --- a/libselinux/src/label.c
>> +++ b/libselinux/src/label.c
>> @@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int
>> backend,
>> rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
>>
>> if ((*initfuncs[backend])(rec, opts, nopts)) {
>> + free(rec->spec_file);
>> free(rec);
>> rec = NULL;
>> }
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c index
>> 1a0c15f..071d902 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -507,6 +507,8 @@ out:
>> return rc;
>> }
>>
>> +static void closef(struct selabel_handle *rec);
>> +
>> static int init(struct selabel_handle *rec, const struct selinux_opt *opts,
>> unsigned n)
>> {
>> @@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const struct
>> selinux_opt *opts,
>>
>> finish:
>> if (status)
>> - free(data->spec_arr);
>> + closef(rec);
>> +
>> return status;
>> }
>>
>> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h index
>> 2492826..a85b9bd 100644
>> --- a/libselinux/src/label_file.h
>> +++ b/libselinux/src/label_file.h
>> @@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data *data, char
>> *buf, int stem_len)
>> }
>> data->stem_arr[num].len = stem_len;
>> data->stem_arr[num].buf = buf;
>> + data->stem_arr[num].from_mmap = 0;
>> data->num_stems++;
>>
>> return num;
>> @@ -425,6 +426,19 @@ static inline int process_line(struct selabel_handle *rec,
>> /* process and store the specification in spec. */
>> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
>> spec_arr[nspec].regex_str = regex;
>> +
>> + /* Convert the type string to a mode format */
>> + spec_arr[nspec].type_str = type;
>> + spec_arr[nspec].mode = 0;
>> +
>> + spec_arr[nspec].lr.ctx_raw = context;
>> +
>> + /*
>> + * bump data->nspecs to cause closef() to cover it in its free
>> + * but do not bump nspec since it's used below.
>> + */
>> + data->nspec++;
>> +
>> if (rec->validating &&
>> compile_regex(data, &spec_arr[nspec], &errbuf)) {
>> COMPAT_LOG(SELINUX_ERROR,
>> @@ -435,9 +449,6 @@ static inline int process_line(struct selabel_handle *rec,
>> return -1;
>> }
>>
>> - /* Convert the type string to a mode format */
>> - spec_arr[nspec].type_str = type;
>> - spec_arr[nspec].mode = 0;
>> if (type) {
>> mode_t mode = string_to_mode(type);
>>
>> @@ -445,14 +456,11 @@ static inline int process_line(struct selabel_handle *rec,
>> COMPAT_LOG(SELINUX_ERROR,
>> "%s: line %u has invalid file type %s\n",
>> path, lineno, type);
>> - errno = EINVAL;
>> return -1;
>> }
>> spec_arr[nspec].mode = mode;
>> }
>>
>> - spec_arr[nspec].lr.ctx_raw = context;
>> -
>> /* Determine if specification has
>> * any meta characters in the RE */
>> spec_hasMetaChars(&spec_arr[nspec]);
>> @@ -460,8 +468,6 @@ static inline int process_line(struct selabel_handle *rec,
>> if (strcmp(context, "<<none>>") && rec->validating)
>> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>>
>> - data->nspec = ++nspec;
>> -
>> return 0;
>> }
>>
>> --
>> 1.9.1
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix memory leaks and uninitialized jump
2015-10-27 17:40 ` Stephen Smalley
@ 2015-10-27 18:05 ` William Roberts
0 siblings, 0 replies; 6+ messages in thread
From: William Roberts @ 2015-10-27 18:05 UTC (permalink / raw
To: Stephen Smalley; +Cc: William Roberts, selinux
[-- Attachment #1: Type: text/plain, Size: 10053 bytes --]
On Oct 27, 2015 10:42 AM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:
>
> Subject: [PATCH] libselinux: label_file: fix memory leaks and
uninitialized jump
>
>
> On 10/26/2015 02:32 PM, william.c.roberts@intel.com wrote:
>>
>> From: William Roberts <william.c.roberts@intel.com>
>>
>> Some error's were reported by valgrind (below) fix them. The test
>> cases on which these leaks were detected:
>
>
> Why do you have leading space in the subject line and every line of the
description?
>
>
>>
>> 1. properly formed file_contexts file.
>> 2. malformed file_contexts file, unknown type.
>> 3. malformed file_contexts file, type that fails on validate
callback.
>> 4. malformed file_contexts file, invalid regex.
>> 5. malformed file_contexts file, invalid mode.
>>
>> ==3819== Conditional jump or move depends on uninitialised value(s)
>> ==3819== at 0x12A682: closef (label_file.c:577)
>> ==3819== by 0x12A196: selabel_close (label.c:163)
>> ==3819== by 0x10A2FD: cleanup (checkfc.c:218)
>> ==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
>> ==3819== by 0x50892A4: exit (exit.c:104)
>> ==3819== by 0x10A231: main (checkfc.c:361)
>> ==3819== Uninitialised value was created by a heap allocation
>> ==3819== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==3819== by 0x4C2CF1F: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==3819== by 0x12BB31: process_file (label_file.h:273)
>> ==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>> ==3819== by 0x10A038: main (checkfc.c:292)
>> ==3819==
>> ==3819==
>> ==3819== HEAP SUMMARY:
>> ==3819== in use at exit: 729 bytes in 19 blocks
>> ==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854
bytes allocated
>> ==3819==
>> ==3819== 81 bytes in 1 blocks are definitely lost in loss record 1
of 2
>> ==3819== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==3819== by 0x50D5839: strdup (strdup.c:42)
>> ==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>> ==3819== by 0x10A038: main (checkfc.c:292)
>> ==3819==
>>
>> ==4238== 40 bytes in 1 blocks are definitely lost in loss record 1
of 6
>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 81 bytes in 1 blocks are definitely lost in loss record 2
of 6
>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x50D5839: strdup (strdup.c:42)
>> ==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 386 bytes in 24 blocks are definitely lost in loss record
3 of 6
>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x50D5889: strndup (strndup.c:45)
>> ==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
>> ==4238== by 0x12B72D: process_file (label_file.h:392)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 648 bytes in 18 blocks are definitely lost in loss record
4 of 6
>> ==4238== at 0x4C2CC70: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
>> ==4238== by 0x117C10: avtab_insert (avtab.c:163)
>> ==4238== by 0x11880A: avtab_read_item (avtab.c:566)
>> ==4238== by 0x118BD3: avtab_read (avtab.c:600)
>> ==4238== by 0x125BDD: policydb_read (policydb.c:3854)
>> ==4238== by 0x109F87: main (checkfc.c:273)
>> ==4238==
>> ==4238== 1,095 bytes in 12 blocks are definitely lost in loss
record 5 of 6
>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
>> ==4238== by 0x12B239: compile_regex (label_file.h:357)
>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>> ==4238==
>> ==4238== 1,296 bytes in 12 blocks are definitely lost in loss
record 6 of 6
>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>> ==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
>> ==4238== by 0x12B25D: compile_regex (label_file.h:366)
>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>> ==4238== by 0x10A038: main (checkfc.c:292)
>>
>> Signed-off-by: William Roberts <william.c.roberts@intel.com>
>> ---
>> libselinux/src/label.c | 1 +
>> libselinux/src/label_file.c | 5 ++++-
>> libselinux/src/label_file.h | 22 ++++++++++++++--------
>> 3 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
>> index c656fda..963bfcb 100644
>> --- a/libselinux/src/label.c
>> +++ b/libselinux/src/label.c
>> @@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int
backend,
>> rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
>>
>> if ((*initfuncs[backend])(rec, opts, nopts)) {
>> + free(rec->spec_file);
>> free(rec);
>> rec = NULL;
>> }
>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
>> index 1a0c15f..071d902 100644
>> --- a/libselinux/src/label_file.c
>> +++ b/libselinux/src/label_file.c
>> @@ -507,6 +507,8 @@ out:
>> return rc;
>> }
>>
>> +static void closef(struct selabel_handle *rec);
>> +
>> static int init(struct selabel_handle *rec, const struct selinux_opt
*opts,
>> unsigned n)
>> {
>> @@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const
struct selinux_opt *opts,
>>
>> finish:
>> if (status)
>> - free(data->spec_arr);
>> + closef(rec);
>> +
>> return status;
>> }
>>
>> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
>> index 2492826..a85b9bd 100644
>> --- a/libselinux/src/label_file.h
>> +++ b/libselinux/src/label_file.h
>> @@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data
*data, char *buf, int stem_len)
>> }
>> data->stem_arr[num].len = stem_len;
>> data->stem_arr[num].buf = buf;
>> + data->stem_arr[num].from_mmap = 0;
>> data->num_stems++;
>>
>> return num;
>> @@ -425,6 +426,19 @@ static inline int process_line(struct
selabel_handle *rec,
>> /* process and store the specification in spec. */
>> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
>> spec_arr[nspec].regex_str = regex;
>> +
>> + /* Convert the type string to a mode format */
>
>
> Technically we should leave this comment line down where it was, at the
point that we actually convert the type string to a mode.
>
>
>> + spec_arr[nspec].type_str = type;
>> + spec_arr[nspec].mode = 0;
>> +
>> + spec_arr[nspec].lr.ctx_raw = context;
>> +
>> + /*
>> + * bump data->nspecs to cause closef() to cover it in its free
>> + * but do not bump nspec since it's used below.
>> + */
>> + data->nspec++;
>> +
>> if (rec->validating &&
>> compile_regex(data, &spec_arr[nspec],
&errbuf)) {
>> COMPAT_LOG(SELINUX_ERROR,
>> @@ -435,9 +449,6 @@ static inline int process_line(struct selabel_handle
*rec,
>> return -1;
>> }
>>
>> - /* Convert the type string to a mode format */
>> - spec_arr[nspec].type_str = type;
>> - spec_arr[nspec].mode = 0;
>> if (type) {
>> mode_t mode = string_to_mode(type);
>>
>> @@ -445,14 +456,11 @@ static inline int process_line(struct
selabel_handle *rec,
>> COMPAT_LOG(SELINUX_ERROR,
>> "%s: line %u has invalid file type
%s\n",
>> path, lineno, type);
>> - errno = EINVAL;
>
>
> Don't want to remove this.
Yes definitely not. That crept in on version 8 of the patch on gerrit you
+1'd. Cannot believe we missed that.
>
>
>> return -1;
>> }
>> spec_arr[nspec].mode = mode;
>> }
>>
>> - spec_arr[nspec].lr.ctx_raw = context;
>> -
>> /* Determine if specification has
>> * any meta characters in the RE */
>> spec_hasMetaChars(&spec_arr[nspec]);
>> @@ -460,8 +468,6 @@ static inline int process_line(struct selabel_handle
*rec,
>> if (strcmp(context, "<<none>>") && rec->validating)
>> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>>
>> - data->nspec = ++nspec;
>> -
>> return 0;
>> }
>>
>>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.
[-- Attachment #2: Type: text/html, Size: 13699 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix memory leaks and uninitialized jump
2015-10-27 17:48 ` Stephen Smalley
@ 2015-10-27 22:18 ` William Roberts
0 siblings, 0 replies; 6+ messages in thread
From: William Roberts @ 2015-10-27 22:18 UTC (permalink / raw
To: Stephen Smalley; +Cc: selinux, William Roberts
[-- Attachment #1: Type: text/plain, Size: 11436 bytes --]
On Oct 27, 2015 10:52 AM, "Stephen Smalley" <sds@tycho.nsa.gov> wrote:
>
> On 10/26/2015 02:42 PM, Roberts, William C wrote:
>>
>> Shouldn't;
>> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>>
>> in process_line() cause a failure? Right now the return code is being
ignored.
>
>
> I think it is historical. Originally we had it bail on error. Red Hat
had problems with that because they would ship a file_contexts file that
had an invalid context or the user would have one bad entry in
file_contexts.local or a local module (maybe due to a policy change that
removed a type, so it might have been valid originally and then became
invalid), and suddenly restorecon/setfiles/rpm/... couldn't label anything
at all, leading to unlabeled files and possibly an unbootable system. So
then we took the error handling to the validate callback, where setfiles or
sefcontext_compile can make it a fatal error but you can still label most
files (the ones that match valid entries) when they get installed via rpm
or using restorecon.
Well it would be nice to reconcile this with Android. What's the plan
there? Runtime option, ifdef, etc?
What's the plan on removing backends unused on Android?
>
>
>>
>> Bill
>>
>>> -----Original Message-----
>>> From: Roberts, William C
>>> Sent: Monday, October 26, 2015 11:33 AM
>>> To: selinux@tycho.nsa.gov
>>> Cc: sds@tycho.nsa.gov; Roberts, William C
>>> Subject: [PATCH] fix memory leaks and uninitialized jump
>>>
>>> From: William Roberts <william.c.roberts@intel.com>
>>>
>>> Some error's were reported by valgrind (below) fix them. The test
>>> cases on which these leaks were detected:
>>>
>>> 1. properly formed file_contexts file.
>>> 2. malformed file_contexts file, unknown type.
>>> 3. malformed file_contexts file, type that fails on validate
callback.
>>> 4. malformed file_contexts file, invalid regex.
>>> 5. malformed file_contexts file, invalid mode.
>>>
>>> ==3819== Conditional jump or move depends on uninitialised value(s)
>>> ==3819== at 0x12A682: closef (label_file.c:577)
>>> ==3819== by 0x12A196: selabel_close (label.c:163)
>>> ==3819== by 0x10A2FD: cleanup (checkfc.c:218)
>>> ==3819== by 0x5089258: __run_exit_handlers (exit.c:82)
>>> ==3819== by 0x50892A4: exit (exit.c:104)
>>> ==3819== by 0x10A231: main (checkfc.c:361)
>>> ==3819== Uninitialised value was created by a heap allocation
>>> ==3819== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==3819== by 0x4C2CF1F: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==3819== by 0x12BB31: process_file (label_file.h:273)
>>> ==3819== by 0x12A2BA: selabel_file_init (label_file.c:522)
>>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>>> ==3819== by 0x10A038: main (checkfc.c:292)
>>> ==3819==
>>> ==3819==
>>> ==3819== HEAP SUMMARY:
>>> ==3819== in use at exit: 729 bytes in 19 blocks
>>> ==3819== total heap usage: 21,126 allocs, 21,107 frees, 923,854
bytes allocated
>>> ==3819==
>>> ==3819== 81 bytes in 1 blocks are definitely lost in loss record 1
of 2
>>> ==3819== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==3819== by 0x50D5839: strdup (strdup.c:42)
>>> ==3819== by 0x12A2A6: selabel_file_init (label_file.c:517)
>>> ==3819== by 0x12A0BB: selabel_open (label.c:88)
>>> ==3819== by 0x10A038: main (checkfc.c:292)
>>> ==3819==
>>>
>>> ==4238== 40 bytes in 1 blocks are definitely lost in loss record 1
of 6
>>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x12A1D2: selabel_file_init (label_file.c:886)
>>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>>> ==4238== by 0x10A038: main (checkfc.c:292)
>>> ==4238==
>>> ==4238== 81 bytes in 1 blocks are definitely lost in loss record 2
of 6
>>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x50D5839: strdup (strdup.c:42)
>>> ==4238== by 0x12A2A6: selabel_file_init (label_file.c:517)
>>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>>> ==4238== by 0x10A038: main (checkfc.c:292)
>>> ==4238==
>>> ==4238== 386 bytes in 24 blocks are definitely lost in loss record
3 of 6
>>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x50D5889: strndup (strndup.c:45)
>>> ==4238== by 0x12CDDF: read_spec_entries (label_support.c:37)
>>> ==4238== by 0x12B72D: process_file (label_file.h:392)
>>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>>> ==4238== by 0x10A038: main (checkfc.c:292)
>>> ==4238==
>>> ==4238== 648 bytes in 18 blocks are definitely lost in loss record
4 of 6
>>> ==4238== at 0x4C2CC70: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x117C9B: avtab_insert_node (avtab.c:105)
>>> ==4238== by 0x117C10: avtab_insert (avtab.c:163)
>>> ==4238== by 0x11880A: avtab_read_item (avtab.c:566)
>>> ==4238== by 0x118BD3: avtab_read (avtab.c:600)
>>> ==4238== by 0x125BDD: policydb_read (policydb.c:3854)
>>> ==4238== by 0x109F87: main (checkfc.c:273)
>>> ==4238==
>>> ==4238== 1,095 bytes in 12 blocks are definitely lost in loss
record 5 of 6
>>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x12D8D1: pcre_compile2 (pcre_compile.c:9217)
>>> ==4238== by 0x12B239: compile_regex (label_file.h:357)
>>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>>> ==4238== by 0x10A038: main (checkfc.c:292)
>>> ==4238==
>>> ==4238== 1,296 bytes in 12 blocks are definitely lost in loss
record 6 of 6
>>> ==4238== at 0x4C2AB80: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-
>>> amd64-linux.so)
>>> ==4238== by 0x13EBE5: pcre_study (pcre_study.c:1565)
>>> ==4238== by 0x12B25D: compile_regex (label_file.h:366)
>>> ==4238== by 0x12B9C7: process_file (label_file.h:429)
>>> ==4238== by 0x12A2BA: selabel_file_init (label_file.c:522)
>>> ==4238== by 0x12A0BB: selabel_open (label.c:88)
>>> ==4238== by 0x10A038: main (checkfc.c:292)
>>>
>>> Signed-off-by: William Roberts <william.c.roberts@intel.com>
>>> ---
>>> libselinux/src/label.c | 1 +
>>> libselinux/src/label_file.c | 5 ++++-
>>> libselinux/src/label_file.h | 22 ++++++++++++++--------
>>> 3 files changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c index
c656fda..963bfcb
>>> 100644
>>> --- a/libselinux/src/label.c
>>> +++ b/libselinux/src/label.c
>>> @@ -338,6 +338,7 @@ struct selabel_handle *selabel_open(unsigned int
>>> backend,
>>> rec->digest = selabel_is_digest_set(opts, nopts, rec->digest);
>>>
>>> if ((*initfuncs[backend])(rec, opts, nopts)) {
>>> + free(rec->spec_file);
>>> free(rec);
>>> rec = NULL;
>>> }
>>> diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index
>>> 1a0c15f..071d902 100644
>>> --- a/libselinux/src/label_file.c
>>> +++ b/libselinux/src/label_file.c
>>> @@ -507,6 +507,8 @@ out:
>>> return rc;
>>> }
>>>
>>> +static void closef(struct selabel_handle *rec);
>>> +
>>> static int init(struct selabel_handle *rec, const struct selinux_opt
*opts,
>>> unsigned n)
>>> {
>>> @@ -580,7 +582,8 @@ static int init(struct selabel_handle *rec, const
struct
>>> selinux_opt *opts,
>>>
>>> finish:
>>> if (status)
>>> - free(data->spec_arr);
>>> + closef(rec);
>>> +
>>> return status;
>>> }
>>>
>>> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index
>>> 2492826..a85b9bd 100644
>>> --- a/libselinux/src/label_file.h
>>> +++ b/libselinux/src/label_file.h
>>> @@ -278,6 +278,7 @@ static inline int store_stem(struct saved_data
*data, char
>>> *buf, int stem_len)
>>> }
>>> data->stem_arr[num].len = stem_len;
>>> data->stem_arr[num].buf = buf;
>>> + data->stem_arr[num].from_mmap = 0;
>>> data->num_stems++;
>>>
>>> return num;
>>> @@ -425,6 +426,19 @@ static inline int process_line(struct
selabel_handle *rec,
>>> /* process and store the specification in spec. */
>>> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
>>> spec_arr[nspec].regex_str = regex;
>>> +
>>> + /* Convert the type string to a mode format */
>>> + spec_arr[nspec].type_str = type;
>>> + spec_arr[nspec].mode = 0;
>>> +
>>> + spec_arr[nspec].lr.ctx_raw = context;
>>> +
>>> + /*
>>> + * bump data->nspecs to cause closef() to cover it in its free
>>> + * but do not bump nspec since it's used below.
>>> + */
>>> + data->nspec++;
>>> +
>>> if (rec->validating &&
>>> compile_regex(data, &spec_arr[nspec],
&errbuf)) {
>>> COMPAT_LOG(SELINUX_ERROR,
>>> @@ -435,9 +449,6 @@ static inline int process_line(struct
selabel_handle *rec,
>>> return -1;
>>> }
>>>
>>> - /* Convert the type string to a mode format */
>>> - spec_arr[nspec].type_str = type;
>>> - spec_arr[nspec].mode = 0;
>>> if (type) {
>>> mode_t mode = string_to_mode(type);
>>>
>>> @@ -445,14 +456,11 @@ static inline int process_line(struct
selabel_handle *rec,
>>> COMPAT_LOG(SELINUX_ERROR,
>>> "%s: line %u has invalid file type
%s\n",
>>> path, lineno, type);
>>> - errno = EINVAL;
>>> return -1;
>>> }
>>> spec_arr[nspec].mode = mode;
>>> }
>>>
>>> - spec_arr[nspec].lr.ctx_raw = context;
>>> -
>>> /* Determine if specification has
>>> * any meta characters in the RE */
>>> spec_hasMetaChars(&spec_arr[nspec]);
>>> @@ -460,8 +468,6 @@ static inline int process_line(struct
selabel_handle *rec,
>>> if (strcmp(context, "<<none>>") && rec->validating)
>>> compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
>>>
>>> - data->nspec = ++nspec;
>>> -
>>> return 0;
>>> }
>>>
>>> --
>>> 1.9.1
>>
>>
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.
>>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.
[-- Attachment #2: Type: text/html, Size: 16245 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-27 22:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-26 18:32 [PATCH] fix memory leaks and uninitialized jump william.c.roberts
2015-10-26 18:42 ` Roberts, William C
2015-10-27 17:48 ` Stephen Smalley
2015-10-27 22:18 ` William Roberts
2015-10-27 17:40 ` Stephen Smalley
2015-10-27 18:05 ` William Roberts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.