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