* [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer
@ 2018-07-01 14:57 Nicolas Iooss
2018-07-01 20:51 ` William Roberts
0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Iooss @ 2018-07-01 14:57 UTC (permalink / raw)
To: selinux
There are many memory leaks in mcstrans. Clean them up in order to
reduce the noise in clang's static analyzer report. Some are remaining,
because they are more complex to fix.
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
mcstrans/src/mcstrans.c | 68 +++++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 12 deletions(-)
diff --git a/mcstrans/src/mcstrans.c b/mcstrans/src/mcstrans.c
index 00fb80856da7..96bdbdff7d8b 100644
--- a/mcstrans/src/mcstrans.c
+++ b/mcstrans/src/mcstrans.c
@@ -708,6 +708,7 @@ append(affix_t **affixes, const char *val) {
err:
log_error("allocation error %s", strerror(errno));
+ free(affix);
return -1;
}
@@ -1517,8 +1518,10 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
} else {
trans = compute_trans_from_raw(range, domain);
if (trans)
- if (add_cache(domain, range, trans) < 0)
+ if (add_cache(domain, range, trans) < 0) {
+ free(range);
return -1;
+ }
}
if (lrange && urange) {
@@ -1526,12 +1529,15 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
if (! ltrans) {
ltrans = compute_trans_from_raw(lrange, domain);
if (ltrans) {
- if (add_cache(domain, lrange, ltrans) < 0)
+ if (add_cache(domain, lrange, ltrans) < 0) {
+ free(range);
return -1;
+ }
} else {
ltrans = strdup(lrange);
if (! ltrans) {
log_error("strdup failed %s", strerror(errno));
+ free(range);
return -1;
}
}
@@ -1541,25 +1547,36 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
if (! utrans) {
utrans = compute_trans_from_raw(urange, domain);
if (utrans) {
- if (add_cache(domain, urange, utrans) < 0)
+ if (add_cache(domain, urange, utrans) < 0) {
+ free(ltrans);
+ free(range);
return -1;
+ }
} else {
utrans = strdup(urange);
if (! utrans) {
log_error("strdup failed %s", strerror(errno));
- return -1;
- }
- }
+ free(ltrans);
+ free(range);
+ return -1;
+ }
+ }
}
if (strcmp(ltrans, utrans) == 0) {
if (asprintf(&trans, "%s", ltrans) < 0) {
log_error("asprintf failed %s", strerror(errno));
+ free(utrans);
+ free(ltrans);
+ free(range);
return -1;
}
} else {
if (asprintf(&trans, "%s-%s", ltrans, utrans) < 0) {
log_error("asprintf failed %s", strerror(errno));
+ free(utrans);
+ free(ltrans);
+ free(range);
return -1;
}
}
@@ -1629,13 +1646,17 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
if (!canonical) {
canonical = compute_trans_from_raw(raw, domain);
if (canonical && strcmp(canonical, range))
- if (add_cache(domain, raw, canonical) < 0)
+ if (add_cache(domain, raw, canonical) < 0) {
+ free(range);
return -1;
+ }
}
if (canonical)
free(canonical);
- if (add_cache(domain, raw, range) < 0)
+ if (add_cache(domain, raw, range) < 0) {
+ free(range);
return -1;
+ }
} else {
log_debug("untrans_context unable to compute raw context %s\n", range);
}
@@ -1650,17 +1671,24 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
if (!canonical) {
canonical = compute_trans_from_raw(lraw, domain);
if (canonical)
- if (add_cache(domain, lraw, canonical) < 0)
+ if (add_cache(domain, lraw, canonical) < 0) {
+ free(lraw);
+ free(range);
return -1;
+ }
}
if (canonical)
free(canonical);
- if (add_cache(domain, lraw, lrange) < 0)
+ if (add_cache(domain, lraw, lrange) < 0) {
+ free(lraw);
+ free(range);
return -1;
+ }
} else {
lraw = strdup(lrange);
if (! lraw) {
log_error("strdup failed %s", strerror(errno));
+ free(range);
return -1;
}
}
@@ -1674,17 +1702,27 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
if (!canonical) {
canonical = compute_trans_from_raw(uraw, domain);
if (canonical)
- if (add_cache(domain, uraw, canonical) < 0)
+ if (add_cache(domain, uraw, canonical) < 0) {
+ free(uraw);
+ free(lraw);
+ free(range);
return -1;
}
+ }
if (canonical)
free(canonical);
- if (add_cache(domain, uraw, urange) < 0)
+ if (add_cache(domain, uraw, urange) < 0) {
+ free(uraw);
+ free(lraw);
+ free(range);
return -1;
+ }
} else {
uraw = strdup(urange);
if (! uraw) {
log_error("strdup failed %s", strerror(errno));
+ free(lraw);
+ free(range);
return -1;
}
}
@@ -1694,11 +1732,17 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
if (strcmp(lraw, uraw) == 0) {
if (asprintf(&raw, "%s", lraw) < 0) {
log_error("asprintf failed %s", strerror(errno));
+ free(uraw);
+ free(lraw);
+ free(range);
return -1;
}
} else {
if (asprintf(&raw, "%s-%s", lraw, uraw) < 0) {
log_error("asprintf failed %s", strerror(errno));
+ free(uraw);
+ free(lraw);
+ free(range);
return -1;
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer
2018-07-01 14:57 [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer Nicolas Iooss
@ 2018-07-01 20:51 ` William Roberts
2018-07-02 18:38 ` Nicolas Iooss
0 siblings, 1 reply; 5+ messages in thread
From: William Roberts @ 2018-07-01 20:51 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: selinux
I see lots of repeating blocks, would it make more sense to goto an
error label and free them then return -1?
On Sun, Jul 1, 2018 at 7:57 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> There are many memory leaks in mcstrans. Clean them up in order to
> reduce the noise in clang's static analyzer report. Some are remaining,
> because they are more complex to fix.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
> mcstrans/src/mcstrans.c | 68 +++++++++++++++++++++++++++++++++--------
> 1 file changed, 56 insertions(+), 12 deletions(-)
>
> diff --git a/mcstrans/src/mcstrans.c b/mcstrans/src/mcstrans.c
> index 00fb80856da7..96bdbdff7d8b 100644
> --- a/mcstrans/src/mcstrans.c
> +++ b/mcstrans/src/mcstrans.c
> @@ -708,6 +708,7 @@ append(affix_t **affixes, const char *val) {
>
> err:
> log_error("allocation error %s", strerror(errno));
> + free(affix);
> return -1;
> }
>
> @@ -1517,8 +1518,10 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
> } else {
> trans = compute_trans_from_raw(range, domain);
> if (trans)
> - if (add_cache(domain, range, trans) < 0)
> + if (add_cache(domain, range, trans) < 0) {
> + free(range);
> return -1;
> + }
> }
>
> if (lrange && urange) {
> @@ -1526,12 +1529,15 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
> if (! ltrans) {
> ltrans = compute_trans_from_raw(lrange, domain);
> if (ltrans) {
> - if (add_cache(domain, lrange, ltrans) < 0)
> + if (add_cache(domain, lrange, ltrans) < 0) {
> + free(range);
> return -1;
> + }
> } else {
> ltrans = strdup(lrange);
> if (! ltrans) {
> log_error("strdup failed %s", strerror(errno));
> + free(range);
> return -1;
> }
> }
> @@ -1541,25 +1547,36 @@ trans_context(const security_context_t incon, security_context_t *rcon) {
> if (! utrans) {
> utrans = compute_trans_from_raw(urange, domain);
> if (utrans) {
> - if (add_cache(domain, urange, utrans) < 0)
> + if (add_cache(domain, urange, utrans) < 0) {
> + free(ltrans);
> + free(range);
> return -1;
> + }
> } else {
> utrans = strdup(urange);
> if (! utrans) {
> log_error("strdup failed %s", strerror(errno));
> - return -1;
> - }
> - }
> + free(ltrans);
> + free(range);
> + return -1;
> + }
> + }
> }
>
> if (strcmp(ltrans, utrans) == 0) {
> if (asprintf(&trans, "%s", ltrans) < 0) {
> log_error("asprintf failed %s", strerror(errno));
> + free(utrans);
> + free(ltrans);
> + free(range);
> return -1;
> }
> } else {
> if (asprintf(&trans, "%s-%s", ltrans, utrans) < 0) {
> log_error("asprintf failed %s", strerror(errno));
> + free(utrans);
> + free(ltrans);
> + free(range);
> return -1;
> }
> }
> @@ -1629,13 +1646,17 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
> if (!canonical) {
> canonical = compute_trans_from_raw(raw, domain);
> if (canonical && strcmp(canonical, range))
> - if (add_cache(domain, raw, canonical) < 0)
> + if (add_cache(domain, raw, canonical) < 0) {
> + free(range);
> return -1;
> + }
> }
> if (canonical)
> free(canonical);
> - if (add_cache(domain, raw, range) < 0)
> + if (add_cache(domain, raw, range) < 0) {
> + free(range);
> return -1;
> + }
> } else {
> log_debug("untrans_context unable to compute raw context %s\n", range);
> }
> @@ -1650,17 +1671,24 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
> if (!canonical) {
> canonical = compute_trans_from_raw(lraw, domain);
> if (canonical)
> - if (add_cache(domain, lraw, canonical) < 0)
> + if (add_cache(domain, lraw, canonical) < 0) {
> + free(lraw);
> + free(range);
> return -1;
> + }
> }
> if (canonical)
> free(canonical);
> - if (add_cache(domain, lraw, lrange) < 0)
> + if (add_cache(domain, lraw, lrange) < 0) {
> + free(lraw);
> + free(range);
> return -1;
> + }
> } else {
> lraw = strdup(lrange);
> if (! lraw) {
> log_error("strdup failed %s", strerror(errno));
> + free(range);
> return -1;
> }
> }
> @@ -1674,17 +1702,27 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
> if (!canonical) {
> canonical = compute_trans_from_raw(uraw, domain);
> if (canonical)
> - if (add_cache(domain, uraw, canonical) < 0)
> + if (add_cache(domain, uraw, canonical) < 0) {
> + free(uraw);
> + free(lraw);
> + free(range);
> return -1;
> }
> + }
> if (canonical)
> free(canonical);
> - if (add_cache(domain, uraw, urange) < 0)
> + if (add_cache(domain, uraw, urange) < 0) {
> + free(uraw);
> + free(lraw);
> + free(range);
> return -1;
> + }
> } else {
> uraw = strdup(urange);
> if (! uraw) {
> log_error("strdup failed %s", strerror(errno));
> + free(lraw);
> + free(range);
> return -1;
> }
> }
> @@ -1694,11 +1732,17 @@ untrans_context(const security_context_t incon, security_context_t *rcon) {
> if (strcmp(lraw, uraw) == 0) {
> if (asprintf(&raw, "%s", lraw) < 0) {
> log_error("asprintf failed %s", strerror(errno));
> + free(uraw);
> + free(lraw);
> + free(range);
> return -1;
> }
> } else {
> if (asprintf(&raw, "%s-%s", lraw, uraw) < 0) {
> log_error("asprintf failed %s", strerror(errno));
> + free(uraw);
> + free(lraw);
> + free(range);
> return -1;
> }
> }
> --
> 2.17.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] 5+ messages in thread
* Re: [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer
2018-07-01 20:51 ` William Roberts
@ 2018-07-02 18:38 ` Nicolas Iooss
2018-07-02 22:49 ` William Roberts
0 siblings, 1 reply; 5+ messages in thread
From: Nicolas Iooss @ 2018-07-02 18:38 UTC (permalink / raw)
To: William Roberts; +Cc: selinux
On Sun, Jul 1, 2018 at 10:51 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> I see lots of repeating blocks, would it make more sense to goto an
> error label and free them then return -1?
Both trans_context() and untrans_context() currently define "char
*ltrans = NULL, *utrans = NULL;" and "char *lrange = NULL, *urange =
NULL;" in the body of a for loop. Introducing an error label at the
end of these functions requires moving these definition outside of the
loop (which could introduce side effects) and introducing the label at
the end of the loop makes the code less readable, IMHO. I guess this
could explain why the current code does not use a "goto error" or
"goto clean" approach and leaks memory where an error occurs.
Anyway, if you are fine with moving the definitions of some variables
(ltrans and utrans for trans_context(), lrange and urange for
untrans_context()), I can write, test and send a new patch with a
"goto error" instead of several free().
Thanks for your review,
Nicolas
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer
2018-07-02 18:38 ` Nicolas Iooss
@ 2018-07-02 22:49 ` William Roberts
2018-07-04 20:12 ` Nicolas Iooss
0 siblings, 1 reply; 5+ messages in thread
From: William Roberts @ 2018-07-02 22:49 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: selinux
On Mon, Jul 2, 2018 at 11:38 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> On Sun, Jul 1, 2018 at 10:51 PM, William Roberts
> <bill.c.roberts@gmail.com> wrote:
>> I see lots of repeating blocks, would it make more sense to goto an
>> error label and free them then return -1?
>
> Both trans_context() and untrans_context() currently define "char
> *ltrans = NULL, *utrans = NULL;" and "char *lrange = NULL, *urange =
> NULL;" in the body of a for loop. Introducing an error label at the
> end of these functions requires moving these definition outside of the
> loop (which could introduce side effects) and introducing the label at
> the end of the loop makes the code less readable, IMHO. I guess this
> could explain why the current code does not use a "goto error" or
> "goto clean" approach and leaks memory where an error occurs.
>
> Anyway, if you are fine with moving the definitions of some variables
> (ltrans and utrans for trans_context(), lrange and urange for
> untrans_context()), I can write, test and send a new patch with a
> "goto error" instead of several free().
This seems fine. I just applied and tested this (finally). Ack.
>
> Thanks for your review,
> Nicolas
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer
2018-07-02 22:49 ` William Roberts
@ 2018-07-04 20:12 ` Nicolas Iooss
0 siblings, 0 replies; 5+ messages in thread
From: Nicolas Iooss @ 2018-07-04 20:12 UTC (permalink / raw)
To: William Roberts; +Cc: selinux
On Tue, Jul 3, 2018 at 12:49 AM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Mon, Jul 2, 2018 at 11:38 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>> On Sun, Jul 1, 2018 at 10:51 PM, William Roberts
>> <bill.c.roberts@gmail.com> wrote:
>>> I see lots of repeating blocks, would it make more sense to goto an
>>> error label and free them then return -1?
>>
>> Both trans_context() and untrans_context() currently define "char
>> *ltrans = NULL, *utrans = NULL;" and "char *lrange = NULL, *urange =
>> NULL;" in the body of a for loop. Introducing an error label at the
>> end of these functions requires moving these definition outside of the
>> loop (which could introduce side effects) and introducing the label at
>> the end of the loop makes the code less readable, IMHO. I guess this
>> could explain why the current code does not use a "goto error" or
>> "goto clean" approach and leaks memory where an error occurs.
>>
>> Anyway, if you are fine with moving the definitions of some variables
>> (ltrans and utrans for trans_context(), lrange and urange for
>> untrans_context()), I can write, test and send a new patch with a
>> "goto error" instead of several free().
>
> This seems fine. I just applied and tested this (finally). Ack.
Thanks. I applied the 3 patches that I sent.
Nicolas
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-04 20:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-01 14:57 [PATCH 1/1] mcstrans: fix memory leaks reported by clang's static analyzer Nicolas Iooss
2018-07-01 20:51 ` William Roberts
2018-07-02 18:38 ` Nicolas Iooss
2018-07-02 22:49 ` William Roberts
2018-07-04 20:12 ` Nicolas Iooss
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.