RE: [PATCH v2 5/6] connect: advertise OS version

From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Date: Jan 17, 2025, 22:47 UTC
Message-ID: <00bb01db6931$d7cd6dc0$87684940$@nexbridge.com>
In-Reply-To: <xmqq4j1xjd2m.fsf@gitster.g>

On January 17, 2025 5:22 PM, Junio C Hamano wrote:

Show 8 quoted lines

>Usman Akinyemi <usmanakinyemi202@gmail.com> writes:
>
>> As some issues that can happen with a Git client can be operating
>> system specific, it can be useful for a server to know which OS a
>> client is using. In the same way it can be useful for a client to know
>> which OS a server is using.
>
>Hmph.  The other end may be running different version of Git, and the

version

>difference of _our_ software is probably more relevant.
>For that matter, they may even be running something entirely different from

our

>software, like Gerrit.  So I am not sure I am convinced that os-version

thing is a good

Show 16 quoted lines

>thing to have with that paragraph.
>
>> Let's introduce a new protocol (`os-version`) allowing Git clients and
>> servers to exchange operating system information. The protocol is
>> controlled by the new `transfer.advertiseOSVersion` config option.
>
>The last sentence is redundant and can safely removed.  The next paragraph
>describes it better than "is controlled by".
>
>> Add the `transfer.advertiseOSVersion` config option to address privacy
>> concerns. It defaults to `true` and can be changed to `false`. When
>> enabled, this option makes clients and servers send each other the OS
>> name (e.g., "Linux" or "Windows"). The information is retrieved using
>> the 'sysname' field of the `uname(2)` system call.
>
>Add "or its equivalent" at the end.

>macOS may have one, but it probably is not quite correct to say that

Windows have

>uname system call (otherwise we wouldn't be emulating it on top of

GetVersion

Show 20 quoted lines

>ourselves).
>
>> However, there are differences between `uname(1)` (command-line
>> utility) and `uname(2)` (system call) outputs on Windows. These
>> discrepancies complicate testing on Windows platforms. For example:
>>   - `uname(1)` output: MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64\
>>   .2024-02-14.20:17.UTC.x86_64
>>   - `uname(2)` output: Windows.10.0.20348
>>
>> On Windows, uname(2) is not actually system-supplied but is instead
>> already faked up by Git itself. We could have overcome the test issue
>> on Windows by implementing a new `uname` subcommand in `test-tool`
>> using uname(2), but except uname(2), which would be tested against
>> itself, there would be nothing platform specific, so it's just simpler
>> to disable the tests on Windows.
>
>OK.
>
>> +transfer.advertiseOSVersion::
>> +	When `true`, the `os-version` capability is advertised by clients

and

>> +	servers. It makes clients and servers send to each other a string
>> +	representing the operating system name, like "Linux" or "Windows".
>> +	This string is retrieved from the `sysname` field of the struct

returned

>> +	by the uname(2) system call. Defaults to true.
>
>Presumably, both ends of the connection independently choose whether they
>enable or disable this variable, so we have 2x2=4 combinations (here,

versions of

>Git before the os-version capability support is introduced behave the same

way as

>an installation with this configuration variable set to false).
>
>And among these four combinations, only one of them results in "send to

each

Show 10 quoted lines

>other", but the description above is fuzzy.
>
>> diff --git a/connect.c b/connect.c
>> index 10fad43e98..6d5792b63c 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -492,6 +492,9 @@ static void send_capabilities(int fd_out, struct
>packet_reader *reader)
>>  	if (server_supports_v2("agent"))
>>  		packet_write_fmt(fd_out, "agent=%s",

git_user_agent_sanitized());

Show 7 quoted lines

>>
>> +	if (server_supports_v2("os-version") &&
>advertise_os_version(the_repository))
>> +		packet_write_fmt(fd_out, "os-version=%s",
>os_version_sanitized());
>
>Not a new problem, because the new code is pretty-much a straight copy from

the

>existing "agent" code, but do we ever use unsanitized versions of

git-user-agent and

>os-version?  If not, I am wondering if we should sanitize immediately when

we

>obtain the raw string and keep it, get rid of _santized() function from the

public API,

>and make anybody calling git_user_agent() and os_version() to get sanitized

safe-

>to-use strings.
>
>I see http.c throws git_user_agent() without doing any sanitization at the

cURL

>library, but it may be a mistake that we may want to fix (outside the scope

of this

>topic).  Since the contrast between the
>os_version() vs the os_version_sanitized() is *new* in this series,

however, we

>probably would want to get it right from the beginning.
>
>So the question is again, do we ever need to use os_version() that is a raw

string

>that may require sanitizing?  I do not think of any offhand.

uname(2) is definitely not portable. uname(1) is almost always available, but there is no guarantee about uname(2). I am not entirely happy having my builds break if having to write one between rc0 and rc1 when this rolls. How is this being handled? os_version() is also not portable. What if we had something that asked for specific elements of the string, by name or id.