Avoid variable shadowing in Tor

changed milestone to %Tor: 0.2.9.x-final

added TorCoreTeam201607 actualpoints::.3 component::core tor/tor easy milestone::Tor: 0.2.9.x-final owner::nickm parent::19379 points::.3 priority::medium refactor resolution::implemented review-group-5 reviewer::teor severity::normal sponsor::S-can status::closed type::defect labels

FWIW GCC and Clang both have -Wshadow which detect these cases.

Working on patches. A quick look at the result of enabling -Wshadow showed tons of warnings so it's going to take some time to fix every instance without breaking other things.

Trac:
Owner: N/A to cypherpunks
Status: new to assigned

Suggestion 1: Think about reviewability as you go. I can easily see this patch becoming huge.

Suggestion 2: Think about automation wherever possible. This test is mostly automatable.

Replying to nickm:

Suggestion 1: Think about reviewability as you go. I can easily see this patch becoming huge.

I would hope to be able to verify the modifications by running the same automated script (see below).

Suggestion 2: Think about automation wherever possible. This test is mostly automatable.

It may help to do this patch in several stages:

automatically rename every shadowed variable
manually cleanup long lines to appease make check-spaces
turn on -Wshadow in the tor makefile

I have noticed several distinct cases of variable shadowing. Each case has its own solution. To make reviewing easier i was thinking of splitting these cases into separate commits.

The cases i have found so far are

redeclaring variables for the same purpose (solution: use the existing variables)
declaring index variables outside for loops (this usually conflicts with subsequent smartlist iterations that use the same index variable names) (solution: declare index variables inside for loops)
using local variable names that match static global variable names (solution: append _local to the local variable names unless renaming them to something more descriptive is appropriate)

I'm unsure how to generate a script that can detect variable shadowing and fix the issues according to the proposed solutions so I'm doing it manually for now.

Please be aware that we might take a long time to review, or reject, a patch that takes a lot of time to manually verify. It's too easy to introduce bugs this way.

Why not try tools like coccinelle, or a scripting language? Most compilers will give you line numbers of shadowing and shadowed variables.

One strategy could be to temporarily change outer and inner names, then check using a compiler. There is a risk that inner variables will accidentally be changed to the outer variable name, but that's the only risk I can see.

Replying to teor:

One strategy could be to temporarily change outer and inner names, then check using a compiler. There is a risk that inner variables will accidentally be changed to the outer variable name, but that's the only risk I can see.

Here is a detailed strategy I thought of, it might not work:

Change outer variable names by appending _o:
- run compiler with -Wshadow and parse the log to create a patch that renames all shadowed declarations to _o, handling SMARTLIST_FOREACH as a special case
- run compiler and rename all undeclared identifiers with _o
Change inner variable names by appending _j:
- run compiler with -Wshadow and rename all shadowed declarations with _j
- run compiler and rename all undeclared identifiers with _j
Revert _o patch
Check for and fix whitespace manually
Remove unnecessary redeclarations manually, if you want to

I noticed a bug here, so I think we should call this 029.

I have a needs_review branch as 'bug18902'.

The unit tests don't pass yet; investigation needed. :)

Trac:
Status: assigned to accepted
Milestone: Tor: 0.2.??? to Tor: 0.2.9.x-final
Actualpoints: N/A to .3
Points: N/A to .3
Owner: cypherpunks to nickm

Trac:
Status: accepted to needs_review

Trac:
Sponsor: N/A to SponsorS-can
Parent: N/A to #19379 (moved)

Trac:
Keywords: easy refactor deleted, easy, refactor, TorCoreTeam201607 added

FWIW i have a branch ready with more fixes. I just had no time left to test it. Should i rebase it on your branch and submit these patches?

If you like, sure!

(I just fixed my branch so that the unit tests pass. Needs review.)

Trac:
Keywords: N/A deleted, review-group-5 added

Trac:
Reviewer: N/A to teor

Actual-review-points: 0.1

1135405 Fix a variable-shadowing bug in check_private_dir Assigns to pw_uid, but doesn't use the value after that. (Some compilers will warn about this.)

I think pw_uid might need to be used in the log message, or the line pw_uid = tor_getpwuid(st.st_uid); should be deleted.

    const struct passwd *pw_uid = NULL;
    char *process_ownername = NULL;

    pw_uid = tor_getpwuid(running_uid);
    process_ownername = pw_uid ? tor_strdup(pw_uid->pw_name) :
      tor_strdup("<unknown>");

    pw_uid = tor_getpwuid(st.st_uid);

    log_warn(LD_FS, "%s is not owned by this user (%s, %d) but by "
        "%s (%d). Perhaps you are running Tor as the wrong user?",
                         dirname, process_ownername, (int)running_uid,
                         pw ? pw->pw_name : "<unknown>", (int)st.st_uid);

(Looking at the rest now.)

In test_addr.c:

-      for (i=0;i<16;++i) {                                       \
+      for (int ii_=0;i<16;++i) {                                 \

Should change all the i's to ii_'s.

The rest looks ok.

Okay, I think I have it sorted out now. Have another look?

Actual-review-points: 0.2

Looks great! Let us improve the code of future developers with yet another automated code check.

Trac:
Status: needs_review to merge_ready

Looks like I merged this on 28 July as a8676b1edecee2d1d472b7f67dd026ef7b084a2f

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 2h 24m

added 2h 24m of time spent

mentioned in issue #19578 (moved)

moved to tpo/core/tor#18902 (closed)

Avoid variable shadowing in Tor

Child items ...

Activity