Windows 8.1 and 10 relays claim to be Windows 8

In Windows 8.1, Microsoft changed the behaviour of the GetVersionEx function so that it returns 6.2 (Windows 8) for applications without a compatibility manifest. (For applications with a compatibility manifest, it returns the version in that manifest.)

https://docs.microsoft.com/en-au/windows/desktop/api/sysinfoapi/nf-sysinfoapi-getversionexa

We should change the version returned by Tor's uname function to "Windows 8 or later".

changed milestone to %Tor: 0.3.4.x-final

added 029-backport 033-backport-unreached 034-backport 035-backport actualpoints::1.0 component::core tor/tor fast-fix milestone::Tor: 0.3.4.x-final owner::teor points::0.2 priority::medium resolution::fixed reviewer::asn severity::normal status::closed type::defect version::tor 0.2.2.34 windows labels

I opened #28097 (moved) to get a more accurate version.

Trac:
Actualpoints: N/A to 0.1
Keywords: N/A deleted, 034-backport, 029-backport, 035-backport, 033-backport added

Reported by Keifer Bly: https://lists.torproject.org/pipermail/tor-relays/2018-October/016466.html

The internal version numbers for Windows versions 8.1 and 10 were sourced from: https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions

My bug28096-029 merges cleanly from maint-0.2.9 to maint-0.3.4. https://github.com/torproject/tor/pull/429

My bug28096-035 merges cleanly from maint-0.3.5 to master. https://github.com/torproject/tor/pull/430

(See https://github.com/teor2345/tor.git for the branches.)

Trac:
Status: new to assigned
Owner: N/A to teor
Points: N/A to 0.2
Version: N/A to Tor: 0.2.2.34
Milestone: Tor: unspecified to Tor: 0.3.6.x-final
Actualpoints: 0.1 to 0.2

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to asn

Patch looks good in principle:

Two things: a) Do you think we can add a unittest for this get_uname() function to test that the new table entries work and will work correctly? b) Should we change Windows 8 to Windows 8 or later even tho the info remains the same? Could there be scripts that break on this?

Marking as needs_revision to get some answers.

Trac:
Status: needs_review to needs_revision

Replying to asn:

Patch looks good in principle:

Two things: a) Do you think we can add a unittest for this get_uname() function to test that the new table entries work and will work correctly?

I don't think we can mock the Windows API function GetVersionEx(), so that makes it hard to unit test get_uname(). But we do call get_uname() as part of test_dir_formats().

We also call get_uname() every time tor is launched:

https://github.com/torproject/tor/blob/67351f672450d5f13754294405243a59ddd86de9/src/app/main/main.c#L606

I would like to print the same string for the unit tests, I'll open a child ticket and update the pull request.

b) Should we change Windows 8 to Windows 8 or later even tho the info remains the same? Could there be scripts that break on this?

Any scripts that expect Windows 8 are already broken, because Tor will return Windows 8 on Windows 8, 8.1, and 10.

These pull requests are unchanged:

Replying to teor:

My bug28096-029 merges cleanly from maint-0.2.9 to maint-0.3.4. https://github.com/torproject/tor/pull/429

My bug28096-035 merges cleanly from maint-0.3.5 to master. https://github.com/torproject/tor/pull/430

(See https://github.com/teor2345/tor.git for the branches.)

I did a minor refactor and implemented #28308 (moved) in bug28096-ticket28308-035: https://github.com/torproject/tor/pull/472

Since it just has test changes and refactoring, we can merge bug28096-ticket28308-035 to 0.3.5 or master.

There's also bug28096-ticket28308-master if we want to merge the new test code to 0.3.6:

https://github.com/torproject/tor/pull/471

Trac:
Status: needs_revision to needs_review

With this code, Appveyor is reporting "Windows 8.1 or later": https://ci.appveyor.com/project/teor2345/tor/builds/20035097/job/6ih2pqwfrf10mtk8#L2741

But the actual version is "Windows Server 2012 R2": https://www.appveyor.com/docs/windows-images-software/#operating-system

Also, the log message is shown every time the unit tests "fork" on Windows.

Looks like I'll need to revise this patch.

Trac:
Status: needs_review to needs_revision

I added the missing Windows server versions, made the "or later" text automatic, and deleted some unreachable code.

The patch is bigger than I'd like to backport, but we can't report the correct Windows version without it. If you'd like, I can reduce the size of the patch by adding the unreachable pre-Windows 2000 code back in.

Here are the branches and pull requests:

My bug28096-029 merges cleanly from maint-0.2.9 to maint-0.3.4. https://github.com/torproject/tor/pull/429

My bug28096-035 merges cleanly from maint-0.3.5 to master. https://github.com/torproject/tor/pull/430

I implemented #28308 (moved) in bug28096-ticket28308-035: https://github.com/torproject/tor/pull/472

Since it just has test changes and refactoring, we can merge bug28096-ticket28308-035 to 0.3.5 or master.

There's still one TODO: I don't know how to just log in the main Windows test process. I'm going to ask nickm how to do that.

Trac:
Status: needs_revision to needs_review

Here's what I am stuck with:

1. I want to test get_uname().
2. I think we can test get_uname() by making the unit tests print the Tor, OS, and library versions when they start. (And that's a nice feature.)
3. But I only want the Windows tests to print this log in the parent test process.
4. The parent/child flag is in tinytest.c, but the tor-specific code is in testing_common.c. How do I get access to the parent/child flag in testing_common.c ?

LGTM but what to do about the TODO: only print this in the main process on Windows,?

Replying to asn:

LGTM but what to do about the TODO: only print this in the main process on Windows,?

nickm will help some time this week.

Tor 0.3.6.x has been renamed to 0.4.0.x.

Trac:
Milestone: Tor: 0.3.6.x-final to Tor: 0.4.0.x-final

Replying to teor:

Replying to asn:

LGTM but what to do about the TODO: only print this in the main process on Windows,?

nickm will help some time this week.

Okay, here's a possibility. On Windows, when tinytest relaunches itself as a subprocess, it passes itself the --RUNNING-FORKED option. You could check for the presence of that option in argv, and use it to decide whether to log the version.

Replying to nickm:

Replying to teor:

Replying to asn:

LGTM but what to do about the TODO: only print this in the main process on Windows,?

nickm will help some time this week.

Okay, here's a possibility. On Windows, when tinytest relaunches itself as a subprocess, it passes itself the --RUNNING-FORKED option. You could check for the presence of that option in argv, and use it to decide whether to log the version.

Ok, but:

4. The parent/child flag is in tinytest.c, but the tor-specific code is in testing_common.c. How do I get access to the parent/child flag in testing_common.c ?

Should I pass argv (or the flag) to testing_common.c, or include tor-specific code in tinytest.c?

Marking this as needs_rev for weekly reviews

Trac:
Status: needs_review to needs_revision

Help with:
       - nickm, I would like help on how to access --RUNNING-FORKED in #28096.
         It seems to require an abstraction layer violation: argv is
in tinytest.c, but tor code is in test_common.c.
         Is there some simple trick I'm missing?
         (So, IMO it would be okay to look at the "v" argument in
main in testing_common.c: it is argv.  Yes, that's a layer violation,
but tinytest is under our control anyway.  Alternatively, we could
extend tinytest, I guess?  I'll be around at the patch party time to
talk more if it's helpful -nickm)