Opened 3 years ago

Closed 3 years ago

#18902 closed defect (implemented)

Avoid variable shadowing in Tor

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, refactor, TorCoreTeam201607, review-group-5
Cc: Actual Points: .3
Parent ID: #19379 Points: .3
Reviewer: teor Sponsor: SponsorS-can

Description

Tor sure does shadow a lot of local and global variables with its block-level variable declarations.

I spot-checked a few of these and they look harmless.

Still, it would be great if we removed them by renaming the variables in the innermost scope. This would avoid confusion, and remove the possibility of bugs where the declaration is removed, and the identifier references the outer scope.

Child Tickets

TicketStatusOwnerSummaryComponent
#19578closednickmIncorrect log message on mismatch ownership in check_private_dirCore Tor/Tor

Change History (21)

comment:1 Changed 3 years ago by cypherpunks

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

Last edited 3 years ago by cypherpunks (previous) (diff)

comment:2 Changed 3 years ago by cypherpunks

Owner: set to cypherpunks
Status: newassigned

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.

comment:3 Changed 3 years ago by nickm

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.

comment:4 in reply to:  3 Changed 3 years ago by teor

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

comment:5 Changed 3 years ago by cypherpunks

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.

comment:6 Changed 3 years ago by teor

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.

comment:7 in reply to:  6 Changed 3 years ago by teor

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

comment:8 Changed 3 years ago by nickm

Actual Points: .3
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Owner: changed from cypherpunks to nickm
Points: .3
Status: assignedaccepted

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

comment:9 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:10 Changed 3 years ago by nickm

Parent ID: #19379
Sponsor: SponsorS-can

comment:11 Changed 3 years ago by nickm

Keywords: TorCoreTeam201607 added

comment:12 Changed 3 years ago by cypherpunks

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?

comment:13 Changed 3 years ago by nickm

If you like, sure!

comment:14 Changed 3 years ago by nickm

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

comment:15 Changed 3 years ago by nickm

Keywords: review-group-5 added

comment:16 Changed 3 years ago by teor

Reviewer: teor

comment:17 Changed 3 years ago by 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.)

comment:18 Changed 3 years ago by teor

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.

comment:19 Changed 3 years ago by nickm

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

comment:20 Changed 3 years ago by teor

Status: needs_reviewmerge_ready

Actual-review-points: 0.2

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

comment:21 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Looks like I merged this on 28 July as a8676b1edecee2d1d472b7f67dd026ef7b084a2f

Note: See TracTickets for help on using tickets.