This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8022

chg: pass copies of some envvars so we can detect py37+ modifications
AbandonedPublic

Authored by spectral on Jan 27 2020, 7:57 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

Python3.7+ will "coerce" some variables in the environment to different values.
Currently this is just LC_CTYPE, which will be coerced to the first of
["C.UTF-8", "C.utf8", "UTF-8"] that works on the platform if LC_CTYPE is
interpreted as "C" (such as by being set to "C", or having LC_ALL set to "C" or
other mechanisms) or is set to an invalid value for the platform (such as
"LC_CTYPE=UTF-8" on Linux).

This is a second attempt at the fix I did in D7550. That fix was based off of an
incorrect reading of the code in Python - I had misinterpreted a #if block that
dealt with Android as applying in all situations, and missed some cases where
this would happen and cause issues.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

spectral created this revision.Jan 27 2020, 7:57 PM
yuja added a subscriber: yuja.Jan 28 2020, 11:10 AM

Another idea. How about sending the true LC_CTYPE value as a command
argument so the env mangling can be reverted?

  1. spawn hg serve --cmdserver chgunix ... with --daemon-postexec setenv:LC_CTYPE=$LC_CTYPE
  2. runservice() overwrites LC_CTYPE to the specified value (undo coercing)
  3. chgserver hashes the restored environ

Since the "coercing" is 100% useless for Mercurial, restoring LC_CTYPE
should be perfectly fine. And unlike setting PYTHONCOERCECLOCALE=0,
Python subprocesses can still see its own "coercing" results for better
or worse.

spectral updated this revision to Diff 19656.Jan 28 2020, 1:56 PM
spectral added a comment.Jan 28 2020, 1:57 PM
In D8022#118325, @yuja wrote:

Another idea. How about sending the true LC_CTYPE value as a command
argument so the env mangling can be reverted?

  1. spawn hg serve --cmdserver chgunix ... with --daemon-postexec setenv:LC_CTYPE=$LC_CTYPE
  2. runservice() overwrites LC_CTYPE to the specified value (undo coercing)
  3. chgserver hashes the restored environ

Since the "coercing" is 100% useless for Mercurial, restoring LC_CTYPE
should be perfectly fine. And unlike setting PYTHONCOERCECLOCALE=0,
Python subprocesses can still see its own "coercing" results for better
or worse.

This would cause a difference in behavior between hg and chg. I don't know how big of an issue that would be.

hg: starts up, python coerces LC_CTYPE, hg spawns a non-python subprocess, LC_CTYPE is set to the coerced value
chg: starts up, python coerces LC_CTYPE, chg fixes it, hg spawns a non-python subprocess, LC_CTYPE is set to the original value (or unset).

Another case where it matters is: LC_CTYPE *is* important for hg when using any curses programs (or really anything that uses the locale module). If you are on a mac and set your region settings to have a region and a primary language that aren't representable using the locale settings (such as region = Brazil, language = English), then LC_CTYPE starts off as "UTF-8" (not "C.UTF-8"). This is allowed on macOS and I think other BSDs, but Linux doesn't like it. If the LC_CTYPE variable is forwarded when sshing to a Linux machine, this breaks curses. I'm sure this wasn't intentional, but the way that Python3.7 coerces the locale, it also happens to fix this particular issue and set it to C.UTF-8, which *does* work on Linux and makes hg commit --interactive with ui.interface=curses work; otherwise it gets a traceback.

yuja added a comment.Jan 28 2020, 7:31 PM
This would cause a difference in behavior between hg and chg. I don't know how big of an issue that would be.
hg: starts up, python coerces LC_CTYPE, hg spawns a non-python subprocess, LC_CTYPE is set to the coerced value
chg: starts up, python coerces LC_CTYPE, chg fixes it, hg spawns a non-python subprocess, LC_CTYPE is set to the original value (or unset).

I think a minor behavior difference is acceptable. I see this is the problem
of Python 3 design "the world is unicode", and IMHO we want to work around
the problem with minimal changes.

PYTHONCOERCECLOCALE=0 was rejected because it may break Python subprocesses
that expects the "coercing" will occur, but restoring the cheated LC_CTYPE
should be fine. The chg behavior will be more correct, and non-Python
subprocesses should work in the original "C" environment.

Another case where it matters is: LC_CTYPE *is* important for hg when using any curses programs (or really anything that uses the locale module). If you are on a mac and set your region settings to have a region and a primary language that aren't representable using the locale settings (such as region = Brazil, language = English), then LC_CTYPE starts off as "UTF-8" (not "C.UTF-8"). This is allowed on macOS and I think other BSDs, but Linux doesn't like it. If the LC_CTYPE variable is forwarded when sshing to a Linux machine, this breaks curses. I'm sure this wasn't intentional, but the way that Python3.7 coerces the locale, it also happens to fix this particular issue and set it to C.UTF-8, which *does* work on Linux and makes `hg commit --interactive` with ui.interface=curses work; otherwise it gets a traceback.

Does the curses depend on the locale environment variable, not on the
setlocale()-ed process state?

Anyway, I think this is an environment issue and it's totally fine for
chg to crash (or abort) if the environment variable is incorrectly set.

spectral added a comment.Jan 29 2020, 2:52 PM
In D8022#118428, @yuja wrote:
This would cause a difference in behavior between hg and chg. I don't know how big of an issue that would be.
hg: starts up, python coerces LC_CTYPE, hg spawns a non-python subprocess, LC_CTYPE is set to the coerced value
chg: starts up, python coerces LC_CTYPE, chg fixes it, hg spawns a non-python subprocess, LC_CTYPE is set to the original value (or unset).

I think a minor behavior difference is acceptable. I see this is the problem
of Python 3 design "the world is unicode", and IMHO we want to work around
the problem with minimal changes.

Getting this implemented "correctly" using daemon_postexec is getting relatively complicated - there's no way to backwards- and forwards-compatibly do this since there's no capabilities mechanism for daemon_postexec. If I blindly do it, it fails when connecting to an older hg saying it doesn't know how to do handle that command.

Much of the stuff being done in this stack is to handle the case with the setenv command (after the chg daemon is started). If we don't care about overwriting the python-modified LC_CTYPE, we can do this quite a bit easier:

In execcmdserver, we copy LC_CTYPE to CHGORIG_LC_CTYPE, export that when starting
In chgserver.py, when doing the initial startup process, check if CHGORIG_LC_CTYPE is in the environment, and overwrite LC_CTYPE with its value.

I'll see how easy that is to implement and send a separate review request once I have it working.

PYTHONCOERCECLOCALE=0 was rejected because it may break Python subprocesses
that expects the "coercing" will occur, but restoring the cheated LC_CTYPE
should be fine. The chg behavior will be more correct, and non-Python
subprocesses should work in the original "C" environment.

Another case where it matters is: LC_CTYPE *is* important for hg when using any curses programs (or really anything that uses the locale module). If you are on a mac and set your region settings to have a region and a primary language that aren't representable using the locale settings (such as region = Brazil, language = English), then LC_CTYPE starts off as "UTF-8" (not "C.UTF-8"). This is allowed on macOS and I think other BSDs, but Linux doesn't like it. If the LC_CTYPE variable is forwarded when sshing to a Linux machine, this breaks curses. I'm sure this wasn't intentional, but the way that Python3.7 coerces the locale, it also happens to fix this particular issue and set it to C.UTF-8, which *does* work on Linux and makes `hg commit --interactive` with ui.interface=curses work; otherwise it gets a traceback.

Does the curses depend on the locale environment variable, not on the
setlocale()-ed process state?

chunkselector (crecord.py:578) calls locale.setlocale(locale.LC_ALL, ''), which calls _setlocale(category, locale), which raises locale.Error: unsupported locale setting. :(

Anyway, I think this is an environment issue and it's totally fine for
chg to crash (or abort) if the environment variable is incorrectly set.

spectral added a comment.Jan 29 2020, 5:24 PM

I'll see how easy that is to implement and send a separate review request once I have it working.

D8039

yuja added a comment.Jan 30 2020, 9:41 AM
Getting this implemented "correctly" using daemon_postexec is getting relatively complicated - there's no way to backwards- and forwards-compatibly do this since there's no capabilities mechanism for daemon_postexec. If I blindly do it, it fails when connecting to an older hg saying it doesn't know how to do handle that command.

For the record, chg doesn't need to support old hg versions. We did once
add --daemon-postexec chdir:/ for example.

In execcmdserver, we copy LC_CTYPE to CHGORIG_LC_CTYPE, export that when starting
In chgserver.py, when doing the initial startup process, check if CHGORIG_LC_CTYPE is in the environment, and overwrite LC_CTYPE with its value.
I'll see how easy that is to implement and send a separate review request once I have it working.

Thanks. I'll review it soon.

spectral updated this revision to Diff 19874.Feb 4 2020, 4:55 PM
spectral abandoned this revision.Feb 10 2020, 2:29 PM

Revision Contents
Changeset List

PathPackages
Mcontrib/chg/chg.c (38 lines)

Diff 19874

contrib/chg/chg.c

hgcmd = (HGPATH); hgcmd = (HGPATH);
#else #else
hgcmd = "hg"; hgcmd = "hg";
#endif #endif
} }
return hgcmd; return hgcmd;
} }
static void putsidechannelenv(const char *envname)
{
char *namebuf, *valbuf;
const char *existing_val = getenv(envname);
size_t existing_val_len = 0, namebuf_len = 0, valbuf_len = 0;
size_t envname_len = strlen(envname);
if (existing_val != NULL) {
existing_val_len = strlen(existing_val);
}
namebuf_len = envname_len + 9;
namebuf = mallocx(namebuf_len);
memcpy(namebuf, "CHGORIG_", 8);
memcpy(namebuf + 8, envname, envname_len + 1);
valbuf_len = existing_val_len + 2;
valbuf = mallocx(valbuf_len);
valbuf[0] = (existing_val == NULL) ? '0' : '1';
if (existing_val != NULL && existing_val_len > 0) {
memcpy(valbuf + 1, existing_val, existing_val_len);
}
valbuf[valbuf_len - 1] = '\0';
if (setenv(namebuf, valbuf, 1) != 0) {
abortmsgerrno("failed to setenv for stored vars");
}
free(valbuf);
free(namebuf);
}
static void execcmdserver(const struct cmdserveropts *opts) static void execcmdserver(const struct cmdserveropts *opts)
{ {
const char *hgcmd = gethgcmd(); const char *hgcmd = gethgcmd();
const char *baseargv[] = { const char *baseargv[] = {
hgcmd, hgcmd,
"serve", "serve",
"--cmdserver", "--cmdserver",
const char **argv = mallocx(sizeof(char *) * argsize); const char **argv = mallocx(sizeof(char *) * argsize);
memcpy(argv, baseargv, sizeof(baseargv)); memcpy(argv, baseargv, sizeof(baseargv));
if (opts->args) { if (opts->args) {
size_t size = sizeof(char *) * opts->argsize; size_t size = sizeof(char *) * opts->argsize;
memcpy(argv + baseargvsize, opts->args, size); memcpy(argv + baseargvsize, opts->args, size);
} }
argv[argsize - 1] = NULL; argv[argsize - 1] = NULL;
char *coerce = getenv("PYTHONCOERCECLOCALE");
if (coerce == NULL || coerce[0] != '0') {
/* Py3 modifies the environment on execution, we need a side
* channel to get an unmodified environment to `hg serve` for
* some variables */
putsidechannelenv("LC_CTYPE");
}
if (putenv("CHGINTERNALMARK=") != 0) if (putenv("CHGINTERNALMARK=") != 0)
abortmsgerrno("failed to putenv"); abortmsgerrno("failed to putenv");
if (execvp(hgcmd, (char **)argv) < 0) if (execvp(hgcmd, (char **)argv) < 0)
abortmsgerrno("failed to exec cmdserver"); abortmsgerrno("failed to exec cmdserver");
free(argv); free(argv);
} }
/* Retry until we can connect to the server. Give up after some time. */ /* Retry until we can connect to the server. Give up after some time. */