Opened 9 months ago

Closed 4 months ago

#28096 closed defect (fixed)

Windows 8.1 and 10 relays claim to be Windows 8

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.34
Severity: Normal Keywords: windows, fast-fix, 029-backport, 034-backport, 035-backport, 033-backport-unreached
Cc: Actual Points: 1.0
Parent ID: Points: 0.2
Reviewer: asn Sponsor:

Description

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".

Child Tickets

TicketStatusOwnerSummaryComponent
#28308closedteorLog the Tor version before running the unit testsCore Tor/Tor

Change History (28)

comment:1 Changed 9 months ago by teor

Actual Points: 0.1
Keywords: 029-backport 033-backport 034-backport 035-backport added

I opened #28097 to get a more accurate version.

comment:3 Changed 9 months ago by teor

Actual Points: 0.10.2
Milestone: Tor: unspecifiedTor: 0.3.6.x-final
Owner: set to teor
Points: 0.2
Status: newassigned
Version: Tor: 0.2.2.34

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

comment:4 Changed 9 months ago by teor

Status: assignedneeds_review

comment:5 Changed 9 months ago by dgoulet

Reviewer: asn

comment:6 Changed 9 months ago by asn

Status: needs_reviewneeds_revision

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.

comment:7 in reply to:  6 Changed 9 months ago by teor

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.

comment:8 in reply to:  3 ; Changed 9 months ago by teor

Status: needs_revisionneeds_review

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 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

comment:9 Changed 9 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:10 in reply to:  8 Changed 9 months ago by teor

Status: needs_revisionneeds_review

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

comment:11 Changed 9 months ago by teor

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 ?

comment:12 Changed 9 months ago by asn

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

comment:13 in reply to:  12 ; Changed 9 months ago by 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.

comment:14 Changed 9 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

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

comment:15 in reply to:  13 ; Changed 8 months ago by 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.

comment:16 in reply to:  15 Changed 8 months ago by teor

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:

  1. 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?

comment:17 Changed 8 months ago by asn

Status: needs_reviewneeds_revision

Marking this as needs_rev for weekly reviews

comment:18 Changed 8 months ago by teor

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)

comment:19 Changed 8 months ago by teor

Actual Points: 0.21.0
Status: needs_revisionneeds_review

I squashed these branches:

bug28096-029-squashed, which merges into maint-0.2.9 to maint-0.3.4.
https://github.com/torproject/tor/pull/510

bug28096-035-squashed, which merges into maint-0.3.5 and master.
https://github.com/torproject/tor/pull/511

I squashed the #28096 parts, and added code to this branch:

ticket28308-035, which merges into maint-0.3.5:
https://github.com/torproject/tor/pull/514

ticket28308-master, which merges into master:
https://github.com/torproject/tor/pull/515

The Windows CI will fail due to #28399, so I made a working branch with the #28399 patch:
https://ci.appveyor.com/project/teor2345/tor/builds/20321178

Version 0, edited 8 months ago by teor (next)

comment:20 Changed 8 months ago by teor

Oops, the working build doesn't show the test logs.
Let's try this one:
https://ci.appveyor.com/project/teor2345/tor/builds/20321569

comment:21 Changed 8 months ago by teor

I see the following versions:

Unit tests for Tor 0.4.0.0-alpha-dev (git-79d3dc2f8b7b3cd8) running on Windows Server 2012 R2 [or later] with Libevent 2.1.8-stable, OpenSSL 1.1.1, Zlib 1.2.11, Liblzma 5.2.4, and Libzstd 1.3.7.
Unit tests for Tor 0.4.0.0-alpha-dev (git-79d3dc2f8b7b3cd8) running on Windows Server 2016 [or later] with Libevent 2.1.8-stable, OpenSSL 1.1.1, Zlib 1.2.11, Liblzma 5.2.4, and Libzstd 1.3.7.

The i686 and x86_64 versions are identical.

https://ci.appveyor.com/project/teor2345/tor/builds/20321569/job/y6k5ldjvkul1k1qe#L2748
https://ci.appveyor.com/project/teor2345/tor/builds/20321569/job/yn8jwg14mnc20xst#L2748

comment:22 Changed 8 months ago by asn

Status: needs_reviewmerge_ready

LGTM! Appveyor seems broken right now but IIUC that's just a transient error and not related to this branch (the old appveyor builds referenced by teor here seem green to me).

comment:23 Changed 8 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.3.4.x-final

Merged to 0.3.5 and forward; marking for possible backport.

comment:24 Changed 5 months ago by teor

Keywords: 033-backport-unreached added; 033-backport removed

These merge_ready tickets did not get backported to 0.3.3, and 0.3.3 is now unsupported.

comment:25 Changed 5 months ago by teor

I closed and re-opened https://github.com/torproject/tor/pull/510 to get CI on a recent merge.

comment:26 Changed 4 months ago by teor

This merge to 0.2.9 conflicts with 0.3.4, here is the 0.3.4 pull request:
https://github.com/torproject/tor/pull/795

comment:27 Changed 4 months ago by teor

The 0.3.4 to 0.3.5 required an "ours" merge, to ignore changes to the old src/common/compat.c. These changes are already in 0.3.5 in the correct file.

comment:28 Changed 4 months ago by teor

Resolution: fixed
Status: merge_readyclosed

#27073, #28096, #25773, #23681, and #23512 were merged to 0.2.9 and later after testing the new maint-0.2.9, maint-0.3.4, and master on CI.

Note: See TracTickets for help on using tickets.