Re: error: 'XXX' may be used uninitialized in this function

[Michal Nowak <mnowak@startmail.com>]

Thank you Ingo very much for your comments. I'll go with your patches.

On 06/03/19 10:37 PM, Ingo Schwarze wrote:
> 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;
> 

I think you duplicated the read.c patch here and you meant 
https://mandoc.bsd.lv/cgi-bin/cvsweb/term.c.diff?r1=1.280&r2=1.281 (I'll 
go with that one instead).

Thanks again!

Michal
--
 To unsubscribe send an email to discuss+unsubscribe@mandoc.bsd.lv