* [PATCH 1/6] semodule_package: do not leak memory when using -u or -s
@ 2017-02-27 20:39 Nicolas Iooss
2017-02-27 20:39 ` [PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null Nicolas Iooss
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
When using -u and -s options, semodule_package's main() allocates
user_extra and seusers to hold the argument values. These allocated
memory blocks are not freed when main() exits, which leads gcc's Address
Sanitizer to report a memory leak. This occurs for example when building
refpolicy base.pp module.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
semodule-utils/semodule_package/semodule_package.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
index e472054826a3..a25daf59f9e7 100644
--- a/semodule-utils/semodule_package/semodule_package.c
+++ b/semodule-utils/semodule_package/semodule_package.c
@@ -257,5 +257,7 @@ int main(int argc, char **argv)
free(file_contexts);
free(outfile);
free(module);
+ free(seusers);
+ free(user_extra);
exit(0);
}
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
@ 2017-02-27 20:39 ` Nicolas Iooss
2017-02-27 20:39 ` [PATCH 3/6] libsemanage: never call memcpy with a NULL value Nicolas Iooss
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsepol/cil/src/cil_resolve_ast.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 5e5298d98e61..87817ca29a5f 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3313,11 +3313,12 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
int rc = SEPOL_OK;
struct cil_args_resolve *args = extra_args;
enum cil_pass pass = 0;
- struct cil_list *ins = args->in_list;
+ struct cil_list *ins;
if (node == NULL || args == NULL) {
goto exit;
}
+ ins = args->in_list;
pass = args->pass;
switch (pass) {
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/6] libsemanage: never call memcpy with a NULL value
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
2017-02-27 20:39 ` [PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null Nicolas Iooss
@ 2017-02-27 20:39 ` Nicolas Iooss
2017-02-27 22:16 ` William Roberts
2017-02-27 20:39 ` [PATCH 4/6] libsemanage/tests: include libsepol headers from $DESTDIR Nicolas Iooss
` (3 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().
Make append_str() do nothing when called with t=NULL.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsemanage/src/semanage_store.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
static char *append_str(char *s, const char *t)
{
size_t s_len = (s == NULL ? 0 : strlen(s));
- size_t t_len = (t == NULL ? 0 : strlen(t));
- char *new_s = realloc(s, s_len + t_len + 1);
+ size_t t_len;
+ char *new_s;
+
+ if (t == NULL) {
+ return s;
+ }
+ t_len = strlen(t);
+ new_s = realloc(s, s_len + t_len + 1);
if (new_s == NULL) {
return NULL;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/6] libsemanage/tests: include libsepol headers from $DESTDIR
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
2017-02-27 20:39 ` [PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null Nicolas Iooss
2017-02-27 20:39 ` [PATCH 3/6] libsemanage: never call memcpy with a NULL value Nicolas Iooss
@ 2017-02-27 20:39 ` Nicolas Iooss
2017-02-27 20:39 ` [PATCH 5/6] mcstrans: do not dereference color_str if it is NULL Nicolas Iooss
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
When building and running tests on a system without SELinux with a
command similar to "make DESTDIR=/tmp/destdir install test", libsemanage
tests fail to build with the following error:
In file included from utilities.h:20:0,
from utilities.c:24:
../src/handle.h:29:26: fatal error: sepol/handle.h: No such file or
directory
#include <sepol/handle.h>
^
Fix this by adding the newly-installed directory under $DESTDIR (using
variable $PREFIX) in the search paths of the compiler.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
.travis.yml | 2 --
libsemanage/tests/Makefile | 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 7d7424459344..fa25d1b336ac 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -105,14 +105,12 @@ script:
- make all -k
# Set up environment variables for the tests
- - export CFLAGS="-I$DESTDIR/usr/include"
- export LD_LIBRARY_PATH="$DESTDIR/usr/lib:$DESTDIR/lib"
- export PATH="$DESTDIR/usr/sbin:$DESTDIR/usr/bin:$DESTDIR/sbin:$DESTDIR/bin:$PATH"
- export PYTHONPATH="$PYSITEDIR"
- export RUBYLIB="$DESTDIR/$($RUBY -e 'puts RbConfig::CONFIG["vendorlibdir"]'):$DESTDIR/$($RUBY -e 'puts RbConfig::CONFIG["vendorarchdir"]')"
# Show variables (to help debugging issues)
- - echo "$CFLAGS"
- echo "$LD_LIBRARY_PATH"
- echo "$PATH"
- echo "$PYTHONPATH"
diff --git a/libsemanage/tests/Makefile b/libsemanage/tests/Makefile
index 3fe7d16f3e72..885b54430155 100644
--- a/libsemanage/tests/Makefile
+++ b/libsemanage/tests/Makefile
@@ -11,7 +11,7 @@ LIBS = ../src/libsemanage.a -L$(LIBDIR) -lselinux -lsepol
EXECUTABLE = libsemanage-tests
CFLAGS += -g -O0 -Wall -W -Wundef -Wmissing-noreturn -Wmissing-format-attribute -Wno-unused-parameter
-INCLUDE = -I../src -I../include
+INCLUDE = -I../src -I../include -I$(PREFIX)/include
LDFLAGS += -lcunit -lbz2 -laudit
OBJECTS = $(SOURCES:.c=.o)
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 5/6] mcstrans: do not dereference color_str if it is NULL
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
` (2 preceding siblings ...)
2017-02-27 20:39 ` [PATCH 4/6] libsemanage/tests: include libsepol headers from $DESTDIR Nicolas Iooss
@ 2017-02-27 20:39 ` Nicolas Iooss
2017-02-27 20:39 ` [PATCH 6/6] libselinux: initialize temp value in SWIG wrapper to prevent freeing garbage Nicolas Iooss
2017-03-01 16:35 ` [PATCH 1/6] semodule_package: do not leak memory when using -u or -s James Carter
5 siblings, 0 replies; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
This bug has been found using clang static analyzer.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
mcstrans/src/mcscolor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mcstrans/src/mcscolor.c b/mcstrans/src/mcscolor.c
index e713da9aea1f..cc6174bba9d5 100644
--- a/mcstrans/src/mcscolor.c
+++ b/mcstrans/src/mcscolor.c
@@ -292,7 +292,7 @@ int raw_color(const security_context_t raw, char **color_str) {
size_t result_size = (N_COLOR * CHARS_PER_COLOR) + 1;
int rc = -1;
- if (!color_str && !*color_str) {
+ if (!color_str || !*color_str) {
return -1;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 6/6] libselinux: initialize temp value in SWIG wrapper to prevent freeing garbage
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
` (3 preceding siblings ...)
2017-02-27 20:39 ` [PATCH 5/6] mcstrans: do not dereference color_str if it is NULL Nicolas Iooss
@ 2017-02-27 20:39 ` Nicolas Iooss
2017-03-01 16:35 ` [PATCH 1/6] semodule_package: do not leak memory when using -u or -s James Carter
5 siblings, 0 replies; 9+ messages in thread
From: Nicolas Iooss @ 2017-02-27 20:39 UTC (permalink / raw)
To: selinux
Currently this Python program triggers a segmentation fault in
libselinux SWIG wrapper:
import selinux
selinux.get_ordered_context_list()
gdb shows that the segmentation fault occurs when freeing some memory:
Reading symbols from python...(no debugging symbols found)...done.
Starting program: /usr/bin/python -c import\
selinux\;selinux.get_ordered_context_list\(\)
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff789a304 in free () from /usr/lib/libc.so.6
(gdb) bt
#0 0x00007ffff789a304 in free () from /usr/lib/libc.so.6
#1 0x00007ffff6011499 in freeconary (con=0x7ffff6ac5d00) at
freeconary.c:14
#2 0x00007ffff6296899 in _wrap_get_ordered_context_list
(self=<optimized out>, args=<optimized out>) at
selinuxswig_wrap.c:6185
#3 0x00007ffff741891f in _PyCFunction_FastCallDict () from
/usr/lib/libpython3.6m.so.1.0
...
SWIG generated the following code for _wrap_get_ordered_context_list():
char ***arg3 = (char ***) 0 ;
char **temp3 ;
arg3 = &temp3;
if (!PyArg_ParseTuple(args, "OO:get_ordered_context_list",&obj0,&obj1))
SWIG_fail;
/* ... */
fail:
if (*arg3) freeconary(*arg3);
If PyArg_ParseTuple fails, freeconary() is called on the value of
"temp3", which has not been initialized. Fix this by initializing temp
to NULL in the SWIG template.
A similar issue exists with security_get_boolean_names(). Fix it too.
This issue has been found using clang's static analyzer, on a system
which uses SWIG 3.0.12.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libselinux/src/selinuxswig.i | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libselinux/src/selinuxswig.i b/libselinux/src/selinuxswig.i
index 687c43bc6d7d..dbdb4c3d72d4 100644
--- a/libselinux/src/selinuxswig.i
+++ b/libselinux/src/selinuxswig.i
@@ -18,7 +18,7 @@
%typedef unsigned mode_t;
%typedef unsigned pid_t;
-%typemap(in, numinputs=0) (char ***names, int *len) (char **temp1, int temp2) {
+%typemap(in, numinputs=0) (char ***names, int *len) (char **temp1=NULL, int temp2) {
$1 = &temp1;
$2 = &temp2;
}
@@ -33,7 +33,7 @@
}
}
-%typemap(in, numinputs=0) (char ***) (char **temp) {
+%typemap(in, numinputs=0) (char ***) (char **temp=NULL) {
$1 = &temp;
}
--
2.11.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/6] libsemanage: never call memcpy with a NULL value
2017-02-27 20:39 ` [PATCH 3/6] libsemanage: never call memcpy with a NULL value Nicolas Iooss
@ 2017-02-27 22:16 ` William Roberts
2017-02-28 4:05 ` William Roberts
0 siblings, 1 reply; 9+ messages in thread
From: William Roberts @ 2017-02-27 22:16 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: selinux
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
On Feb 27, 2017 12:42, "Nicolas Iooss" <nicolas.iooss@m4x.org> wrote:
clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().
Make append_str() do nothing when called with t=NULL.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsemanage/src/semanage_store.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_
store.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
static char *append_str(char *s, const char *t)
{
size_t s_len = (s == NULL ? 0 : strlen(s));
- size_t t_len = (t == NULL ? 0 : strlen(t));
- char *new_s = realloc(s, s_len + t_len + 1);
+ size_t t_len;
+ char *new_s;
+
+ if (t == NULL) {
+ return s;
+ }
+ t_len = strlen(t);
+ new_s = realloc(s, s_len + t_len + 1);
Overflow possibility here?
if (new_s == NULL) {
return NULL;
}
--
2.11.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.
[-- Attachment #2: Type: text/html, Size: 2541 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/6] libsemanage: never call memcpy with a NULL value
2017-02-27 22:16 ` William Roberts
@ 2017-02-28 4:05 ` William Roberts
0 siblings, 0 replies; 9+ messages in thread
From: William Roberts @ 2017-02-28 4:05 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: selinux
[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]
On Feb 27, 2017 2:16 PM, "William Roberts" <bill.c.roberts@gmail.com> wrote:
On Feb 27, 2017 12:42, "Nicolas Iooss" <nicolas.iooss@m4x.org> wrote:
clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().
Make append_str() do nothing when called with t=NULL.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsemanage/src/semanage_store.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_sto
re.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
static char *append_str(char *s, const char *t)
{
size_t s_len = (s == NULL ? 0 : strlen(s));
- size_t t_len = (t == NULL ? 0 : strlen(t));
- char *new_s = realloc(s, s_len + t_len + 1);
+ size_t t_len;
+ char *new_s;
+
+ if (t == NULL) {
+ return s;
+ }
+ t_len = strlen(t);
+ new_s = realloc(s, s_len + t_len + 1);
Overflow possibility here?
I guess since s and t lengths come from strlen() and the architectures we
worry about running code on, overflowing would be pretty impossible here.
if (new_s == NULL) {
return NULL;
}
--
2.11.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.
[-- Attachment #2: Type: text/html, Size: 3514 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/6] semodule_package: do not leak memory when using -u or -s
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
` (4 preceding siblings ...)
2017-02-27 20:39 ` [PATCH 6/6] libselinux: initialize temp value in SWIG wrapper to prevent freeing garbage Nicolas Iooss
@ 2017-03-01 16:35 ` James Carter
5 siblings, 0 replies; 9+ messages in thread
From: James Carter @ 2017-03-01 16:35 UTC (permalink / raw)
To: Nicolas Iooss, selinux
On 02/27/2017 03:39 PM, Nicolas Iooss wrote:
> When using -u and -s options, semodule_package's main() allocates
> user_extra and seusers to hold the argument values. These allocated
> memory blocks are not freed when main() exits, which leads gcc's Address
> Sanitizer to report a memory leak. This occurs for example when building
> refpolicy base.pp module.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
I applied all six of these patches.
Thanks,
Jim
> ---
> semodule-utils/semodule_package/semodule_package.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
> index e472054826a3..a25daf59f9e7 100644
> --- a/semodule-utils/semodule_package/semodule_package.c
> +++ b/semodule-utils/semodule_package/semodule_package.c
> @@ -257,5 +257,7 @@ int main(int argc, char **argv)
> free(file_contexts);
> free(outfile);
> free(module);
> + free(seusers);
> + free(user_extra);
> exit(0);
> }
>
--
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-03-01 16:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-27 20:39 [PATCH 1/6] semodule_package: do not leak memory when using -u or -s Nicolas Iooss
2017-02-27 20:39 ` [PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null Nicolas Iooss
2017-02-27 20:39 ` [PATCH 3/6] libsemanage: never call memcpy with a NULL value Nicolas Iooss
2017-02-27 22:16 ` William Roberts
2017-02-28 4:05 ` William Roberts
2017-02-27 20:39 ` [PATCH 4/6] libsemanage/tests: include libsepol headers from $DESTDIR Nicolas Iooss
2017-02-27 20:39 ` [PATCH 5/6] mcstrans: do not dereference color_str if it is NULL Nicolas Iooss
2017-02-27 20:39 ` [PATCH 6/6] libselinux: initialize temp value in SWIG wrapper to prevent freeing garbage Nicolas Iooss
2017-03-01 16:35 ` [PATCH 1/6] semodule_package: do not leak memory when using -u or -s James Carter
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.