Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#17076 closed enhancement (fixed)

Improve coverage on src/or/config.c (options_validate)

Reported by: rjunior Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, 028-triaged, TorCoreTeam201602
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor: SponsorS

Description

The changes are in the branch "options_test"

https://github.com/twstrike/tor_for_patching/tree/options_test

Child Tickets

TicketTypeStatusOwnerSummary
#18095defectclosedUnit test integer precision warnings on 32 bit clang targets
#18096defectclosedtest_options_validate__control fails on Windows because there are no ControlSockets

Attachments (1)

leaks_options (696.1 KB) - added by nickm 4 years ago.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 4 years ago by rjunior

Summary: Improve coverage on src/or/config.cImprove coverage on src/or/config.c (options_validate)

comment:2 Changed 4 years ago by rjunior

Status: newneeds_review

comment:3 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:4 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:5 Changed 4 years ago by nickm

Sponsor: SponsorS

comment:6 Changed 4 years ago by nickm

Points: small

mark these testing tickets in needs_review as 'small work remaining'

comment:7 Changed 4 years ago by olabini

I have just pushed some updates to the branch that fixes all whitespace issues.

comment:8 Changed 4 years ago by nickm

Severity: Normal
Status: needs_reviewneeds_revision

There seems to be a large number of memory leaks here. Is that something you can fix?

I'm attacking the valgrind logs.

Changed 4 years ago by nickm

Attachment: leaks_options added

comment:9 Changed 4 years ago by olabini

Sorry about that, I will take a look over the next few days hopefully.

comment:10 Changed 4 years ago by olabini

I have now pushed a fix to the branch in question that fixes all memory leaks in our tests. There is one exception, where one test triggers OpenSSL to cache some ECC information.

There is also a small leak in the options_validate test that was there before - this has to do with the log capture, that uses a log callback. This log callback gets registered with a duplicated string for the filename, but I couldn't really figure out the right place to free this.

comment:11 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

comment:12 in reply to:  10 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

Replying to olabini:

I have now pushed a fix to the branch in question that fixes all memory leaks in our tests. There is one exception, where one test triggers OpenSSL to cache some ECC information.

This is probably ok.

There is also a small leak in the options_validate test that was there before - this has to do with the log capture, that uses a log callback. This log callback gets registered with a duplicated string for the filename, but I couldn't really figure out the right place to free this.

Tinytest has a setup/teardown mechanism that could be used to deregister the callback and free the string.

Alternately, it can be deregistered/freed at the end of every test function.

(Or are you having trouble accessing the duplicated string from the tests? If it's static, it can be marked STATIC, and placed in the *_PRIVATE section of the header.)

comment:13 Changed 4 years ago by olabini

As I said, the leak was there before. I can definitely try to fix it in any of those ways, but it's likely to be a bit more invasive than just accessing a STATIC.

comment:14 Changed 4 years ago by olabini

Status: needs_revisionneeds_review

I have fixed this in the most appropriate way I could figure, by adding a small helper function to cleanly remove the log callback. Hopefully this should all be fine now.

comment:15 in reply to:  14 Changed 4 years ago by cypherpunks

Replying to olabini:

I have fixed this in the most appropriate way I could figure, by adding a small helper function to cleanly remove the log callback. Hopefully this should all be fine now.

Couldn't you just have used mark_logs_temp() and close_temp_logs()?

comment:16 Changed 4 years ago by olabini

Thank you, I didn't find those functions when looking for something suitable. The branch is now updated to use those instead.

comment:17 in reply to:  16 Changed 4 years ago by cypherpunks

Replying to olabini:

Thank you, I didn't find those functions when looking for something suitable. The branch is now updated to use those instead.

I would also swap clear_log_messages and close_temp_logs to be sure no messages are sent to the callback function in between those calls (which would then reallocated the messages smartlist, leading to another memory leak). It is unlikely to happen but it is better to be correct now than later IMO.

comment:18 Changed 4 years ago by olabini

Of course, should have thought of that. Done now.

comment:19 Changed 4 years ago by teor

Thanks for these fixes, the code looks good, but I want (someone) to rebase onto master and run the tests before we merge these changes.

comment:20 Changed 4 years ago by olabini

I have now rebased the code onto master (as of d062baac86c824f8a3ddfead3a8c88b3f87068c1) and also updated the tests that had subjected to bitrot. This should merge cleanly now. I've also rerun valgrind and ensured no memory leaks were introduced.

comment:21 Changed 4 years ago by teor

Thanks, let's get it merged then!

comment:22 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks okay to me; merged it!

comment:23 Changed 4 years ago by teor

I think these changes may have introduced integer precision warnings on 32 bit clang targets.
Please see #18095 for details.

comment:24 Changed 4 years ago by teor

These unit tests fail on Windows, because Windows does not have unix sockets.
See #18096.

comment:25 Changed 4 years ago by olabini

Resolution: implemented
Status: closedreopened

Ouch, will take a look at those immediately.

comment:26 Changed 4 years ago by olabini

Status: reopenedneeds_review

I've pushed two commits to this branch, fixing both #18095 and #18096.

comment:27 Changed 4 years ago by teor

Code review:

  • #18095: the int ops commit looks fine
  • #18096: the unix socket commit looks ok, you could check for an error message when we don't have unix sockets, but I'm not sure that gets us much.

Let's get this merged to make our buildbots happy.

comment:28 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Lgtm; merged!

comment:29 Changed 4 years ago by teor

Resolution: fixed
Status: closedreopened

The way these tests are implemented makes it hard to add new options.

Testing an option requires a developer to specify a set of options that silence all potential warnings or rejections before the option being tested. Several tests count a specific number of warnings, or locate a warning by its position in the log.

This means that adding new option can break existing tests. And writing unit tests for a new option requires significant effort.

I suggest we revise these tests to:

  • never rely on a specific option count or log message position
    • instead, search the entire log for the message, and assert that it's been found somewhere in it
  • never rely on the developer to choose a set of default options that validate correctly
    • instead, supply a set of default options that validate correctly at the top of the file, and use them throughout the file, overriding individual options as needed

comment:30 in reply to:  29 Changed 4 years ago by teor

Replying to teor:

  • never rely on the developer to choose a set of default options that validate correctly
    • instead, supply a set of default options that validate correctly at the top of the file, and use them throughout the file, overriding individual options as needed

Ideally, these options would validate without warnings, as well.

comment:31 Changed 4 years ago by teor

Please see the commit "Add mock_saved_log_position to unit tests" for an order-independent check for a log message.

The following commit "Add unit tests for ClientUseIPv[4,6] and ClientPreferIPv6[OR,Dir]Port" provides examples of using that function to search for a log message.

It would be great if we could search the entire log for log messages in the rest of the unit tests, rather than expecting them in particular positions.

comment:32 Changed 4 years ago by olabini

Where are those commits committed? I can't see them in the main repository.

comment:33 Changed 4 years ago by nickm

Branches that Teor mentions will be in his public repository at https://github.com/teor2345/tor.git

comment:34 Changed 4 years ago by olabini

OK, first, here is a new branch: https://github.com/twstrike/tor_for_patching/tree/options_validate_second_round

This branch changes all the places in options_validate that checks at a specific log index to check whether the log message exist or not.

comment:35 Changed 4 years ago by olabini

When it comes to this:

never rely on the developer to choose a set of default options that validate correctly

instead, supply a set of default options that validate correctly at the top of the file, and use them throughout the file, overriding individual options as needed

I'm not sure I understand exactly what you mean. The problem is that the current setup is tuned to reach as many branches as possible.

The real solution here is actually to separate out options_validate into smaller functions that can be tested in isolation, instead of testing the full options_validate - the goal of testing all branches in that function was as a step to make it possible to refactor the function into something more manageable.

comment:36 in reply to:  35 Changed 4 years ago by teor

Replying to olabini:

When it comes to this:

never rely on the developer to choose a set of default options that validate correctly

instead, supply a set of default options that validate correctly at the top of the file, and use them throughout the file, overriding individual options as needed

I'm not sure I understand exactly what you mean. The problem is that the current setup is tuned to reach as many branches as possible.

Oops, sorry, the branch I didn't mention at all was feature17840-v11-merged in #17840 / #18132.

Tuning the unit tests to reach as many branches as possible is not our highest priority. Writing robust unit tests that test the important invariants in the implementation (and only important invariants) is more important.

Covering all the branches makes it really hard to change anything, or refactor anything, because everything has to behave exactly like it did before, even down to the level of unimportant behaviour like the order of the checks in options_validate().

While the fact that the check is performed is important, the fact that it happens before another check is rarely significant. Sometimes it could be significant if the check is in a tightly-coupled group of checks on the same or similar options.

That said, the current setup could be modified to reduce the number of warning messages issued. We normally prefer our unit tests to complete without warnings, in this case, they should complete with the least number of warnings necessary to perform the test. Some of the unit tests had many unnecessary warnings because appropriate default options weren't set. This shouldn't affect coverage (much), as long as those options are tested elsewhere.

The real solution here is actually to separate out options_validate into smaller functions that can be tested in isolation, instead of testing the full options_validate - the goal of testing all branches in that function was as a step to make it possible to refactor the function into something more manageable.

I agree, it's incredibly long and needs to be refactored. But we don't need 100% coverage to do that, we need coverage of all the important invariants in the code.

comment:37 Changed 4 years ago by teor

(We'd normally see warnings in unit tests, but because these tests capture warnings using the test mock functions, we don't.)

comment:38 in reply to:  34 Changed 4 years ago by teor

Replying to olabini:

OK, first, here is a new branch: https://github.com/twstrike/tor_for_patching/tree/options_validate_second_round

This branch changes all the places in options_validate that checks at a specific log index to check whether the log message exist or not.

Thanks for this change, it looks good. Let's get it merged. (This might make a mess of #17840's unit tests, we'll work that out when we get to it.)

comment:39 Changed 4 years ago by teor

Status: reopenedneeds_review

comment:40 Changed 4 years ago by teor

Keywords: TorCoreTeam201602 added

I cherry-picked ae6c5e8 from olabini's branch, and added three other correctness fixes:

  • correctly handle errors when parsing options
  • make all unit tests independent of log message order and count
  • fix potential buffer overrun in snprintf by using tor_snprintf
    • (On some platforms, snprintf automatically nul-terminates its string, and then returns the number of characters it wanted to write. Writing a nul to the position returned by snprintf is an error.)

Please see my branch options_validate_second_round at https://github.com/teor2345/tor.git
(Sorry for forgetting to include my github URL last time.)

comment:41 Changed 4 years ago by olabini

Looks good to me, thanks!

comment:42 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I made an options_validate_second_round_cleaned branch, minus the two commits f0933b9 and 6290547, which I think were an accident. Then, merged it. Thanks!

comment:43 Changed 4 years ago by teor

Added #18337 with a suggested fix for some of these unit tests being slow: they appear to be quick when the network is down, so my guess is that they're using the local resolver on non-existent domains.

Note: See TracTickets for help on using tickets.