* Re: mdocml version 1.12.0 available [not found] ` <20111011145641.GA25314@britannica.bec.de> @ 2011-10-13 0:50 ` Ingo Schwarze 2011-10-13 9:55 ` Ulrich Spörlein 0 siblings, 1 reply; 6+ messages in thread From: Ingo Schwarze @ 2011-10-13 0:50 UTC (permalink / raw) To: Joerg Sonnenberger; +Cc: tech Hi Joerg, Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200: > Attached is the report from clang -analyze. Food for thought ;) Thanks for sending these. Below is a patch to improve style at a few places, none of these are bugs, though. No regressions according to my tests. A few more reports are valid, but i suggest to keep those redundant initializations because it is non-trivial to figure out that all branches actually set a value, so it's safer to initialize up front, especially in view of potential future code changes. Two complaints might be bogus: > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > if (n->prev && MDOC_It == n->prev->tok) { > ^ > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > if (n->prev && MDOC_It == n->prev->tok) > ^ I have no idea why clang thinks n might be NULL here. Do you understand that? Or is this just a false positive? Yours, Ingo Index: man_validate.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v retrieving revision 1.47 diff -u -p -r1.47 man_validate.c --- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47 +++ man_validate.c 13 Oct 2011 00:04:33 -0000 @@ -209,12 +209,12 @@ check_text(CHKARGS) { char *cp, *p; - cp = p = n->string; - for (cp = p; NULL != (p = strchr(p, '\t')); p++) { - if (MAN_LITERAL & m->flags) - continue; + if (MAN_LITERAL & m->flags) + return; + + cp = n->string; + for (p = cp; NULL != (p = strchr(p, '\t')); p++) man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB); - } } #define INEQ_DEFINE(x, ineq, name) \ @@ -470,7 +470,6 @@ post_UC(CHKARGS) const char *p, *s; n = n->child; - n = m->last->child; if (NULL == n || MAN_TEXT != n->type) p = bsd_versions[0]; Index: mdoc_html.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_html.c,v retrieving revision 1.63 diff -u -p -r1.63 mdoc_html.c --- mdoc_html.c 9 Oct 2011 22:10:51 -0000 1.63 +++ mdoc_html.c 13 Oct 2011 00:04:33 -0000 @@ -499,7 +499,7 @@ mdoc_root_post(MDOC_ARGS) print_otag(h, TAG_COL, 1, tag); print_otag(h, TAG_COL, 1, tag); - t = print_otag(h, TAG_TBODY, 0, NULL); + print_otag(h, TAG_TBODY, 0, NULL); tt = print_otag(h, TAG_TR, 0, NULL); Index: mdoc_macro.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_macro.c,v retrieving revision 1.69 diff -u -p -r1.69 mdoc_macro.c --- mdoc_macro.c 18 Sep 2011 15:54:48 -0000 1.69 +++ mdoc_macro.c 13 Oct 2011 00:04:34 -0000 @@ -1428,18 +1428,15 @@ blk_part_exp(MACRO_PROT_ARGS) /* Clean-up to leave in a consistent state. */ - if (NULL == head) { + if (NULL == head) if ( ! mdoc_head_alloc(m, line, ppos, tok)) return(0); - head = m->last; - } if (NULL == body) { if ( ! rew_sub(MDOC_HEAD, m, tok, line, ppos)) return(0); if ( ! mdoc_body_alloc(m, line, ppos, tok)) return(0); - body = m->last; } /* Standard appending of delimiters. */ Index: mdoc_validate.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_validate.c,v retrieving revision 1.95 diff -u -p -r1.95 mdoc_validate.c --- mdoc_validate.c 18 Sep 2011 15:54:48 -0000 1.95 +++ mdoc_validate.c 13 Oct 2011 00:04:34 -0000 @@ -541,12 +541,11 @@ check_text(struct mdoc *m, int ln, int p { char *cp; - cp = p; - for (cp = p; NULL != (p = strchr(p, '\t')); p++) { - if (MDOC_LITERAL & m->flags) - continue; + if (MDOC_LITERAL & m->flags) + return; + + for (cp = p; NULL != (p = strchr(p, '\t')); p++) mdoc_pmsg(m, ln, pos + (int)(p - cp), MANDOCERR_BADTAB); - } } static int Index: term_ps.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/term_ps.c,v retrieving revision 1.18 diff -u -p -r1.18 term_ps.c --- term_ps.c 18 Sep 2011 15:54:48 -0000 1.18 +++ term_ps.c 13 Oct 2011 00:04:34 -0000 @@ -488,8 +488,7 @@ pspdf_alloc(char *outopts) pagey = 356; } else if (2 != sscanf(pp, "%ux%u", &pagex, &pagey)) fprintf(stderr, "%s: Unknown paper\n", pp); - } else if (NULL == pp) - pp = "letter"; + } /* * This MUST be defined before any PNT2AFM or AFM2PNT @@ -576,7 +575,7 @@ ps_printf(struct termp *p, const char *f ps_growbuf(p, PS_BUFSLOP); pos = (int)p->ps->psmargcur; - len = vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap); + vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap); va_end(ap); -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available 2011-10-13 0:50 ` mdocml version 1.12.0 available Ingo Schwarze @ 2011-10-13 9:55 ` Ulrich Spörlein 2011-10-15 15:02 ` Ingo Schwarze 0 siblings, 1 reply; 6+ messages in thread From: Ulrich Spörlein @ 2011-10-13 9:55 UTC (permalink / raw) To: tech On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote: > Hi Joerg, > > Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200: > > > Attached is the report from clang -analyze. Food for thought ;) > > Thanks for sending these. > Below is a patch to improve style at a few places, > none of these are bugs, though. > No regressions according to my tests. > > A few more reports are valid, but i suggest to keep those redundant > initializations because it is non-trivial to figure out that all > branches actually set a value, so it's safer to initialize up > front, especially in view of potential future code changes. > > Two complaints might be bogus: > > > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > > if (n->prev && MDOC_It == n->prev->tok) { > > ^ > > /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > > if (n->prev && MDOC_It == n->prev->tok) > > ^ > > I have no idea why clang thinks n might be NULL here. > Do you understand that? > Or is this just a false positive? print_bvspace could have been called with n being NULL. Perhaps the HTML report is little bit more helpful: https://www.spoerlein.net/scan-build/mdocml/2011-10-13-1/report-qHe1Tf.html#EndPath hth Uli -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available 2011-10-13 9:55 ` Ulrich Spörlein @ 2011-10-15 15:02 ` Ingo Schwarze 2011-10-15 18:28 ` Joerg Sonnenberger 0 siblings, 1 reply; 6+ messages in thread From: Ingo Schwarze @ 2011-10-15 15:02 UTC (permalink / raw) To: tech Hi Ulrich, Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200: > On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote: >> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200: >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') >>> if (n->prev && MDOC_It == n->prev->tok) { >>> ^ >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') >>> if (n->prev && MDOC_It == n->prev->tok) >>> ^ >> I have no idea why clang thinks n might be NULL here. >> Do you understand that? >> Or is this just a false positive? > print_bvspace could have been called with n being NULL. No, i just checked all callers. All callers access n before calling print_bvspace, so it's a false positive. > Perhaps the HTML > report is little bit more helpful: > https://www.spoerlein.net/scan-build/mdocml/2011-10-13-1/report-qHe1Tf.html#EndPath Not really, but i consider this settled all the same. Thanks, Ingo -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available 2011-10-15 15:02 ` Ingo Schwarze @ 2011-10-15 18:28 ` Joerg Sonnenberger 2011-10-16 10:35 ` Ingo Schwarze 0 siblings, 1 reply; 6+ messages in thread From: Joerg Sonnenberger @ 2011-10-15 18:28 UTC (permalink / raw) To: tech On Sat, Oct 15, 2011 at 05:02:49PM +0200, Ingo Schwarze wrote: > Hi Ulrich, > > Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200: > > On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote: > >> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200: > > >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > >>> if (n->prev && MDOC_It == n->prev->tok) { > >>> ^ > >>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') > >>> if (n->prev && MDOC_It == n->prev->tok) > >>> ^ > > >> I have no idea why clang thinks n might be NULL here. > >> Do you understand that? > >> Or is this just a false positive? > > > print_bvspace could have been called with n being NULL. > > No, i just checked all callers. > All callers access n before calling print_bvspace, > so it's a false positive. That's not clear from the code flow and given that there are explicit checks for NULL around, it punts. You could make this assumptions clear with an assert. Joerg -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available 2011-10-15 18:28 ` Joerg Sonnenberger @ 2011-10-16 10:35 ` Ingo Schwarze 2011-10-16 11:01 ` Kristaps Dzonsons 0 siblings, 1 reply; 6+ messages in thread From: Ingo Schwarze @ 2011-10-16 10:35 UTC (permalink / raw) To: tech Hi Joerg, Joerg Sonnenberger wrote on Sat, Oct 15, 2011 at 08:28:16PM +0200: > On Sat, Oct 15, 2011 at 05:02:49PM +0200, Ingo Schwarze wrote: >> Ulrich Spörlein wrote on Thu, Oct 13, 2011 at 11:55:25AM +0200: >>> On Thu, 2011-10-13 at 02:50:45 +0200, Ingo Schwarze wrote: >>>> Joerg Sonnenberger wrote on Tue, Oct 11, 2011 at 04:56:41PM +0200: >>>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:617:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') >>>>> if (n->prev && MDOC_It == n->prev->tok) { >>>>> ^ >>>>> /home/joerg/work/NetBSD/cvs/src/external/bsd/mdocml/bin/mandoc/../../dist/mdoc_term.c:611:7: warning: Access to field 'prev' results in a dereference of a null pointer (loaded from variable 'n') >>>>> if (n->prev && MDOC_It == n->prev->tok) >>>>> ^ >>>> I have no idea why clang thinks n might be NULL here. >>>> Do you understand that? >>>> Or is this just a false positive? >>> print_bvspace could have been called with n being NULL. >> No, i just checked all callers. >> All callers access n before calling print_bvspace, >> so it's a false positive. > That's not clear from the code flow and given that there are explicit > checks for NULL around, it punts. You could make this assumptions clear > with an assert. Hm, i see, that won't do any harm. Patch updated; i still see no regressions. OK? Ingo Index: man_validate.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v retrieving revision 1.47 diff -u -p -r1.47 man_validate.c --- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47 +++ man_validate.c 16 Oct 2011 10:28:55 -0000 @@ -209,12 +209,12 @@ check_text(CHKARGS) { char *cp, *p; - cp = p = n->string; - for (cp = p; NULL != (p = strchr(p, '\t')); p++) { - if (MAN_LITERAL & m->flags) - continue; + if (MAN_LITERAL & m->flags) + return; + + cp = n->string; + for (p = cp; NULL != (p = strchr(p, '\t')); p++) man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB); - } } #define INEQ_DEFINE(x, ineq, name) \ @@ -470,7 +470,6 @@ post_UC(CHKARGS) const char *p, *s; n = n->child; - n = m->last->child; if (NULL == n || MAN_TEXT != n->type) p = bsd_versions[0]; Index: mdoc_html.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_html.c,v retrieving revision 1.63 diff -u -p -r1.63 mdoc_html.c --- mdoc_html.c 9 Oct 2011 22:10:51 -0000 1.63 +++ mdoc_html.c 16 Oct 2011 10:28:55 -0000 @@ -499,7 +499,7 @@ mdoc_root_post(MDOC_ARGS) print_otag(h, TAG_COL, 1, tag); print_otag(h, TAG_COL, 1, tag); - t = print_otag(h, TAG_TBODY, 0, NULL); + print_otag(h, TAG_TBODY, 0, NULL); tt = print_otag(h, TAG_TR, 0, NULL); Index: mdoc_macro.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_macro.c,v retrieving revision 1.69 diff -u -p -r1.69 mdoc_macro.c --- mdoc_macro.c 18 Sep 2011 15:54:48 -0000 1.69 +++ mdoc_macro.c 16 Oct 2011 10:28:56 -0000 @@ -1428,18 +1428,15 @@ blk_part_exp(MACRO_PROT_ARGS) /* Clean-up to leave in a consistent state. */ - if (NULL == head) { + if (NULL == head) if ( ! mdoc_head_alloc(m, line, ppos, tok)) return(0); - head = m->last; - } if (NULL == body) { if ( ! rew_sub(MDOC_HEAD, m, tok, line, ppos)) return(0); if ( ! mdoc_body_alloc(m, line, ppos, tok)) return(0); - body = m->last; } /* Standard appending of delimiters. */ Index: mdoc_term.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_term.c,v retrieving revision 1.137 diff -u -p -r1.137 mdoc_term.c --- mdoc_term.c 20 Sep 2011 09:02:18 -0000 1.137 +++ mdoc_term.c 16 Oct 2011 10:28:56 -0000 @@ -580,6 +580,8 @@ print_bvspace(struct termp *p, { const struct mdoc_node *nn; + assert(n); + term_newln(p); if (MDOC_Bd == bl->tok && bl->norm->Bd.comp) Index: mdoc_validate.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/mdoc_validate.c,v retrieving revision 1.95 diff -u -p -r1.95 mdoc_validate.c --- mdoc_validate.c 18 Sep 2011 15:54:48 -0000 1.95 +++ mdoc_validate.c 16 Oct 2011 10:28:56 -0000 @@ -541,12 +541,11 @@ check_text(struct mdoc *m, int ln, int p { char *cp; - cp = p; - for (cp = p; NULL != (p = strchr(p, '\t')); p++) { - if (MDOC_LITERAL & m->flags) - continue; + if (MDOC_LITERAL & m->flags) + return; + + for (cp = p; NULL != (p = strchr(p, '\t')); p++) mdoc_pmsg(m, ln, pos + (int)(p - cp), MANDOCERR_BADTAB); - } } static int Index: term_ps.c =================================================================== RCS file: /cvs/src/usr.bin/mandoc/term_ps.c,v retrieving revision 1.18 diff -u -p -r1.18 term_ps.c --- term_ps.c 18 Sep 2011 15:54:48 -0000 1.18 +++ term_ps.c 16 Oct 2011 10:28:57 -0000 @@ -488,8 +488,7 @@ pspdf_alloc(char *outopts) pagey = 356; } else if (2 != sscanf(pp, "%ux%u", &pagex, &pagey)) fprintf(stderr, "%s: Unknown paper\n", pp); - } else if (NULL == pp) - pp = "letter"; + } /* * This MUST be defined before any PNT2AFM or AFM2PNT @@ -576,7 +575,7 @@ ps_printf(struct termp *p, const char *f ps_growbuf(p, PS_BUFSLOP); pos = (int)p->ps->psmargcur; - len = vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap); + vsnprintf(&p->ps->psmarg[pos], PS_BUFSLOP, fmt, ap); va_end(ap); -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: mdocml version 1.12.0 available 2011-10-16 10:35 ` Ingo Schwarze @ 2011-10-16 11:01 ` Kristaps Dzonsons 0 siblings, 0 replies; 6+ messages in thread From: Kristaps Dzonsons @ 2011-10-16 11:01 UTC (permalink / raw) To: tech > Index: man_validate.c > =================================================================== > RCS file: /cvs/src/usr.bin/mandoc/man_validate.c,v > retrieving revision 1.47 > diff -u -p -r1.47 man_validate.c > --- man_validate.c 18 Sep 2011 15:54:48 -0000 1.47 > +++ man_validate.c 16 Oct 2011 10:28:55 -0000 > @@ -209,12 +209,12 @@ check_text(CHKARGS) > { > char *cp, *p; > > - cp = p = n->string; > - for (cp = p; NULL != (p = strchr(p, '\t')); p++) { > - if (MAN_LITERAL& m->flags) > - continue; > + if (MAN_LITERAL& m->flags) > + return; > + > + cp = n->string; > + for (p = cp; NULL != (p = strchr(p, '\t')); p++) > man_pmsg(m, n->line, (int)(p - cp), MANDOCERR_BADTAB); > - } Ingo, Joerg, I'm fine with these---thanks! (Gah, what was I thinking!?) Take care, Kristaps -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-16 11:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <4E90B130.3080008@bsd.lv>
[not found] ` <20111011145641.GA25314@britannica.bec.de>
2011-10-13 0:50 ` mdocml version 1.12.0 available Ingo Schwarze
2011-10-13 9:55 ` Ulrich Spörlein
2011-10-15 15:02 ` Ingo Schwarze
2011-10-15 18:28 ` Joerg Sonnenberger
2011-10-16 10:35 ` Ingo Schwarze
2011-10-16 11:01 ` Kristaps Dzonsons
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).