[Geany-devel] RFC: uninitialized variable in theoretical execution path.

[Colomban Wendling]

Le 06/08/2011 16:02, Daniel Marjamäki a écrit :
> Hi!

Hi,

> I wonder what you think about fixing "uninitialized variable" issues
> for theoretical execution paths.
> 
> What I mean with "theoretical" is that it is not assumed that
> parameters and global variables are restricted to the "possible"
> values - they could also have "impossible values".
> 
> An example:
> 
> An uninitialized variable "y" can theoretically be used in
> src/printing.c at line 753.
> 
> Here is the code in shortened format:
> 
> 514     gdouble x, y;
> 
> 561     while (count < dinfo->lines_per_page)
>        {
> 686                     cairo_get_current_point(cr, &x, &y);
>        }
> 
> 742     if (printing_prefs.print_line_numbers)
>        {
> 752             cairo_line_to(cr, (dinfo->max_line_number_margin *
> dinfo->font_width) + 1,
>                        y + dinfo->line_height); /* y is last added
> line, we reuse it */
>        }
> 
> The variable "y" is initialized in the loop. If the loop is skipped
> the variable will not be initialized. Therefore if
> dinfo->lines_per_page is 0 and printing_prefs.print_line_numbers is
> true then there will be a "uninitialized variable" issue.
> 
> I guess there is an assumption that dinfo->lines_per_page will always
> be greater than 0. Perhaps that assumption is ok.

I think it's fine to fix those, at least nobody would wonder once again
about it. And I like some sort of defensive programming, and it's
something cheap in term of performances, so it's OK to "fix".

> PS.
> 
> I wrote a simple Cppcheck plugin that searches for "uninitialized
> variable" in "theoretical execution paths". It will assume that all
> conditions can always be both true/false.

Interesting, can it be found somewhere?

> I assume that most of the reports are false positives. But if you are
> interested.. here is what it says for geany/src:
> 
> [src/dialogs.c:257]: (information) Suspecting that uninitialized
> variable is used: iter_parent

False positive, always set in the switch above it (there's even a default)

> [src/dialogs.c:684]: (information) Suspecting that uninitialized
> variable is used: resp

False positive, resp is unconditionally set in the body of the
do...while loop, and check in the condition.

> [src/dialogs.c:740]: (information) Suspecting that uninitialized
> variable is used: title

Again a false positive because of a switch

> [src/dialogs.c:774]: (information) Suspecting that uninitialized
> variable is used: args
> [src/dialogs.c:1663]: (information) Suspecting that uninitialized
> variable is used: args
> [src/dialogs.c:1686]: (information) Suspecting that uninitialized
> variable is used: args
> [src/dialogs.c:1713]: (information) Suspecting that uninitialized
> variable is used: args

False positives, it's actually the initializations using va_start().

> [src/editor.c:2852]: (information) Suspecting that uninitialized
> variable is used: style_comment
> [src/editor.c:4890]: (information) Suspecting that uninitialized
> variable is used: mode

False positives because of a switch

> [src/encodings.c:643]: (information) Suspecting that uninitialized
> variable is used: charset

False positive, it's set or never reached because of a continue;

> [src/filetypes.c:854]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/filetypes.c:1395]: (information) Suspecting that uninitialized
> variable is used: result
> [src/keybindings.c:1543]: (information) Suspecting that uninitialized
> variable is used: item

False positives because of a switch

> [src/log.c:68]: (information) Suspecting that uninitialized variable
> is used: args
> [src/msgwindow.c:269]: (information) Suspecting that uninitialized
> variable is used: args
> [src/msgwindow.c:341]: (information) Suspecting that uninitialized
> variable is used: args
> [src/msgwindow.c:399]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/printing.c:753]: (information) Suspecting that uninitialized
> variable is used: y

Fixed, initialized to 0.0

> [src/socket.c:685]: (information) Suspecting that uninitialized
> variable is used: newline

Unconditionally initialized in the do...while body, checked in the cond

> [src/stash.c:767]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/stash.c:1057]: (information) Suspecting that uninitialized
> variable is used: setting

False positive: either set or the code won't be reached. I refactored a
bit the code to be more clear that the setting will always be set.

> [src/symbols.c:205]: (information) Suspecting that uninitialized
> variable is used: tag_type

False positive because of a switch

> [src/symbols.c:636]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/symbols.c:1917]: (information) Suspecting that uninitialized
> variable is used: fn_style

switch

> [src/templates.c:577]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/tools.c:127]: (information) Suspecting that uninitialized
> variable is used: stock_id

False positive (not obvious though)

> [src/ui_utils.c:148]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/ui_utils.c:885]: (information) Suspecting that uninitialized
> variable is used: widget_name

switch

> [src/ui_utils.c:1842]: (information) Suspecting that uninitialized
> variable is used: args
> [src/ui_utils.c:2365]: (information) Suspecting that uninitialized
> variable is used: a
> [src/utils.c:1365]: (information) Suspecting that uninitialized
> variable is used: a
> [src/utils.c:2090]: (information) Suspecting that uninitialized
> variable is used: args

va_start()

> [src/win32.c:713]: (information) Suspecting that uninitialized
> variable is used: t
> [src/win32.c:705]: (information) Suspecting that uninitialized
> variable is used: title

switch

> There could be a real issue in one of those reports but I don't know
> how likely that is.
> 
> I am in the process right now to analyse the results. If I see genuine
> problems I'll report them.

There was no real issues except for the one you spoke of in the mail's
body. Basically the false positives was va_start()-initialized and
variables initialized in all switches' cases.

Though, thanks for reviewing the code :)


Regards,
Colomban