| Message ID | 20180701145739.23309-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
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
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 >
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
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; } }
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(-)