discuss@mandoc.bsd.lv
 help / color / mirror / Atom feed
From: Ingo Schwarze <schwarze@usta.de>
To: Michal Nowak <mnowak@startmail.com>
Cc: discuss@mandoc.bsd.lv
Subject: Re: error: 'XXX' may be used uninitialized in this function
Date: Mon, 3 Jun 2019 22:37:14 +0200	[thread overview]
Message-ID: <20190603203714.GA59058@athene.usta.de> (raw)
In-Reply-To: <5a8f2439-c566-c9e2-f796-86dccbd697f9@startmail.com>

Hi Michael,

Michal Nowak wrote on Mon, Jun 03, 2019 at 05:28:28PM +0200:

> I work on updating mandoc to 1.14.5 on illumos

Appreciated, thank you!

> and there are few errors (warnings treated as errors) when building
> mandoc with GCC 4.4.4 and 7.4.0:

[text reordered]
> In all cases I was able to eliminate those errors by initializing 
> respective variables to NULL.

In general, i object to blindly initializing variables without
considering whether it makes sense logically; that is dangerous
behaviour on part of the programmer because it can hide errors in
the code - both existing errors and errors introduced later.

> It seems that none of those errors were 
> addressed after the 1.14.5 release.

No, because no are errors: they are all false positives.

> In function 'print_mdoc_node':
> error: 'act' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>     if (act->post == NULL || n->flags & NODE_ENDED)
>         ~~~^~~~~~

The pointer act is assigned to in the default case of the previous
n->type switch, and it is used only in the default case of the
second n->type switch.

That said, the only way this pointer is used is by dereferencing it.
So (pointlessly) initializing it does no harm because in case a bug
is introduced later, it changes an uninitialized pointer access into
a NULL pointer access which is potentially less dangerous.  So in this
particular case, your suggestion to hide the error is OK.

> In function 'mparse_buf_r':
> /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/read.c:147: 
> error: 'lastln' may be used uninitialized in this function [-Wuninitialized]

Another false positive: lastln is only used when firstln is already
set, and whenever firstln gets set, lastln is also set at the same
time.  That said, it is logically clear from the purpose of the
variable that initializing lastln is harmless in this case.  Logically,
lastln *should* be NULL as long as firstln is NULL.

> In function 'term_fill':
> /export/home/newman/ws/oi-userland/components/openindiana/illumos-gate/illumos-gate/usr/src/cmd/mandoc/term.c:286:24: 
> error: 'vn' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>      if (breakline || vn > vtarget)

And yet another false positive.  The variable vn is assigned to in
all three cases of the inner switch and only used immediately afterwards.

Apparently, the compiler isn't smart enough to see that the cases
of the inner switch are exhaustive.  Then again, there is no harm
in saying so explicitly.

So, i committed the three patches shown below.

Can you confirm that your gcc no longer complains?

Yours,
  Ingo


Log Message:
-----------
Initialize the local variable "act" in print_mdoc_node().
While there is no bug, it helps clarity, and it is also safer in this
particular code because in case a bug gets introduced later, accessing
a NULL pointer is less dangerous than accessing an uninitialized pointer.

Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4 
and 7.4.0 on illumos throw -Wuninitialized false positives.

Modified Files:
--------------
    mandoc:
        mdoc_term.c

Revision Data
-------------
Index: mdoc_term.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/mdoc_term.c,v
retrieving revision 1.372
retrieving revision 1.373
diff -Lmdoc_term.c -Lmdoc_term.c -u -p -r1.372 -r1.373
--- mdoc_term.c
+++ mdoc_term.c
@@ -352,6 +352,7 @@ print_mdoc_node(DECL_ARGS)
 	 * produce output.  Note that some pre-handlers do so.
 	 */
 
+	act = NULL;
 	switch (n->type) {
 	case ROFFT_TEXT:
 		if (n->flags & NODE_LINE) {


Log Message:
-----------
Initialize the local variable "lastln" in mparse_buf_r().
While there is no bug, it logically makes sense given the meaning
of the variable that lastln is NULL as long as firstln is NULL.

Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4
and 7.4.0 on illumos throw -Wuninitialized false positives.

Modified Files:
--------------
    mandoc:
        read.c

Revision Data
-------------
Index: read.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/read.c,v
retrieving revision 1.212
retrieving revision 1.213
diff -Lread.c -Lread.c -u -p -r1.212 -r1.213
--- read.c
+++ read.c
@@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct
 	ln.sz = 256;
 	ln.buf = mandoc_malloc(ln.sz);
 	ln.next = NULL;
-	firstln = loop = NULL;
+	firstln = lastln = loop = NULL;
 	lnn = curp->line;
 	pos = 0;
 	inloop = 0;


Log Message:
-----------
Initialize the local variable "lastln" in mparse_buf_r().
While there is no bug, it logically makes sense given the meaning
of the variable that lastln is NULL as long as firstln is NULL.

Michal Nowak <mnowak at startmail dot com> reported that gcc 4.4.4
and 7.4.0 on illumos throw -Wuninitialized false positives.

Modified Files:
--------------
    mandoc:
        read.c

Revision Data
-------------
Index: read.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/read.c,v
retrieving revision 1.212
retrieving revision 1.213
diff -Lread.c -Lread.c -u -p -r1.212 -r1.213
--- read.c
+++ read.c
@@ -157,7 +157,7 @@ mparse_buf_r(struct mparse *curp, struct
 	ln.sz = 256;
 	ln.buf = mandoc_malloc(ln.sz);
 	ln.next = NULL;
-	firstln = loop = NULL;
+	firstln = lastln = loop = NULL;
 	lnn = curp->line;
 	pos = 0;
 	inloop = 0;
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv

  reply	other threads:[~2019-06-03 20:37 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03 15:28 Michal Nowak
2019-06-03 20:37 ` Ingo Schwarze [this message]
2019-06-04  4:53   ` Michal Nowak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190603203714.GA59058@athene.usta.de \
    --to=schwarze@usta.de \
    --cc=discuss@mandoc.bsd.lv \
    --cc=mnowak@startmail.com \
    --subject='Re: error: '\''XXX'\'' may be used uninitialized in this function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).