Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Zac Medico]

On 5/26/20 10:32 AM, Ulrich Mueller wrote:
>> On Tue, 26 May 2020, Zac Medico wrote:
> 
>> On 5/26/20 12:48 AM, Michał Górny wrote:
>>> On Mon, 2020-05-25 at 21:31 -0700, Zac Medico wrote:
 Since variables like A and AA can contain extremely large values which
 may trigger E2BIG errors during attempts to execute subprocesses, delay
 export until the last moment, and unexport when appropriate.
>>>
>>> Please don't.  This will only hide the problem from developers who will
>>> unknowingly commit broken ebuilds and cause users of alternative package
>>> managers to suffer.
> 
>> We've seen in https://bugs.gentoo.org/719202 that developers can already
>> do that with existing versions of portage, since the failure can be
>> dependent on USE configuration.
> 
> We have seen in bug 719202 that A has exceeded _SC_ARG_MAX which is
> 128 KiB.
> 
> However, your commit message mentions E2BIG which will trigger at a much
> larger value, namely 2 MiB. We are far away from reaching that limit in
> any ebuild.
> 
> These are separate issues (although related), so I think we should make
> it very clear about which one we're talking.
> 
> Ulrich
> 
If we want to differentiate between these things then that's fine with me,
however, I have not seen an error other than errno 7 which I thought
corresponded to E2BIG. Test case:

$ python -c "import os, subprocess; os.environ['A'] = 131072 * ' '; 
subprocess.check_call(['echo', 'hello world'])"
Traceback (most recent call last):
  File "", line 1, in 
  File "/usr/lib64/python3.6/subprocess.py", line 306, in check_call
retcode = call(*popenargs, **kwargs)
  File "/usr/lib64/python3.6/subprocess.py", line 287, in call
with Popen(*popenargs, **kwargs) as p:
  File "/usr/lib64/python3.6/subprocess.py", line 729, in __init__
restore_signals, start_new_session)
  File "/usr/lib64/python3.6/subprocess.py", line 1364, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
OSError: [Errno 7] Argument list too long: 'echo'
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Zac Medico]

On 5/26/20 10:16 AM, Alec Warner wrote:
> On Tue, May 26, 2020 at 9:46 AM Zac Medico  > wrote:
> 
> On 5/26/20 1:43 AM, Alec Warner wrote:
> > On Mon, May 25, 2020 at 9:34 PM Zac Medico  
> > >> wrote:
> >
> >     Since variables like A and AA can contain extremely large
> values which
> >     may trigger E2BIG errors during attempts to execute
> subprocesses, delay
> >     export until the last moment, and unexport when appropriate.
> >
> >
> > So I think if you want to do this because PMS says:
> >  AA should not be visible in EAPI > 3.
> >  A should only be visible in src_*, pkg_nofetch.
> >
> > That part of the patch makes sense to me. The part that is
> confusing to
> > me is the 'delay' part; can you explain that further? When you say
> > "delay until the last moment" what do you mean by that and what
> value is
> > it delivering?
> 
> If we export an environment variable which contains an extremely large
> value, then there's a vulnerability in execve which causes it to fail
> with an E2BIG error. Since A and AA values can easily grow large enough
> to trigger this vulnerability, portage can protect itself from execve
> failures by delaying the export until the moment that it hands control
> to the ebuild phase.
> 
> 
> > Is it simply that we don't export these variables on the python side,
> > and we only use them in the shell portion?
> 
> That's correct. Here's a test case which demonstrates the E2BIG error,
> and shows that 'export -n A' can suppress it:
> 
> 
> I've run similar tests, but I'm less excited by this work around. I
> think its reasonable to work toward eventually not exporting A and AA
> (the latter already done in EAPIs > 3). Then when ebuilds encounter
> problems, we can tell authors "Upgrade to EAPI X" (where X is >3 or >=8;
> in the potential case of A.) Having a hard limit on A for EAPIs 0-7 and
> a hard limit on AA for EAPIs 0-3 seems perfectly fine and we should
> expect authors to refactor (as texlive did) in order to meet the
> existing API limitations.
> 
> Basically unless there are more practical use cases today, the delaying
> of export seems like a feature no one will use and added complexity. I
> dunno how large the go-mod use cases are yet; but I myself have not seen
> any with this many deps.
> 
> -A


I'm in no rush to merge this patch. I wanted to create it as a proof of
concept, so that we'd be prepared for a future EAPI where the size of A
is practically unlimited. We only have to adjust the ___eapi_exports_A
function to have a working EAPI extension.
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Ulrich Mueller]

> On Tue, 26 May 2020, Zac Medico wrote:

> On 5/26/20 12:48 AM, Michał Górny wrote:
>> On Mon, 2020-05-25 at 21:31 -0700, Zac Medico wrote:
>>> Since variables like A and AA can contain extremely large values which
>>> may trigger E2BIG errors during attempts to execute subprocesses, delay
>>> export until the last moment, and unexport when appropriate.
>> 
>> Please don't.  This will only hide the problem from developers who will
>> unknowingly commit broken ebuilds and cause users of alternative package
>> managers to suffer.

> We've seen in https://bugs.gentoo.org/719202 that developers can already
> do that with existing versions of portage, since the failure can be
> dependent on USE configuration.

We have seen in bug 719202 that A has exceeded _SC_ARG_MAX which is
128 KiB.

However, your commit message mentions E2BIG which will trigger at a much
larger value, namely 2 MiB. We are far away from reaching that limit in
any ebuild.

These are separate issues (although related), so I think we should make
it very clear about which one we're talking.

Ulrich


signature.asc
Description: PGP signature

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Alec Warner]

On Tue, May 26, 2020 at 9:46 AM Zac Medico  wrote:

> On 5/26/20 1:43 AM, Alec Warner wrote:
> > On Mon, May 25, 2020 at 9:34 PM Zac Medico  > > wrote:
> >
> > Since variables like A and AA can contain extremely large values
> which
> > may trigger E2BIG errors during attempts to execute subprocesses,
> delay
> > export until the last moment, and unexport when appropriate.
> >
> >
> > So I think if you want to do this because PMS says:
> >  AA should not be visible in EAPI > 3.
> >  A should only be visible in src_*, pkg_nofetch.
> >
> > That part of the patch makes sense to me. The part that is confusing to
> > me is the 'delay' part; can you explain that further? When you say
> > "delay until the last moment" what do you mean by that and what value is
> > it delivering?
>
> If we export an environment variable which contains an extremely large
> value, then there's a vulnerability in execve which causes it to fail
> with an E2BIG error. Since A and AA values can easily grow large enough
> to trigger this vulnerability, portage can protect itself from execve
> failures by delaying the export until the moment that it hands control
> to the ebuild phase.
>

> > Is it simply that we don't export these variables on the python side,
> > and we only use them in the shell portion?
>
> That's correct. Here's a test case which demonstrates the E2BIG error,
> and shows that 'export -n A' can suppress it:
>

I've run similar tests, but I'm less excited by this work around. I think
its reasonable to work toward eventually not exporting A and AA (the latter
already done in EAPIs > 3). Then when ebuilds encounter problems, we can
tell authors "Upgrade to EAPI X" (where X is >3 or >=8; in the potential
case of A.) Having a hard limit on A for EAPIs 0-7 and a hard limit on AA
for EAPIs 0-3 seems perfectly fine and we should expect authors to refactor
(as texlive did) in order to meet the existing API limitations.

Basically unless there are more practical use cases today, the delaying of
export seems like a feature no one will use and added complexity. I dunno
how large the go-mod use cases are yet; but I myself have not seen any with
this many deps.

-A


>
> $ A=$(dd if=/dev/zero bs=1M count=1 | tr '\0' ' ')
> 10+0 records in
> 10+0 records out
> 10485760 bytes (10 MB, 10 MiB) copied, 0.086557 s, 121 MB/s
> $ echo ${#A}
> 10485760
> $ export A
> $ ls
> bash: /bin/ls: Argument list too long
> $ export -n A
> $ /bin/echo hello world
> hello world


> --
> Thanks,
> Zac
>
>

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Zac Medico]

On 5/26/20 12:48 AM, Michał Górny wrote:
> On Mon, 2020-05-25 at 21:31 -0700, Zac Medico wrote:
>> Since variables like A and AA can contain extremely large values which
>> may trigger E2BIG errors during attempts to execute subprocesses, delay
>> export until the last moment, and unexport when appropriate.
>>
> 
> Please don't.  This will only hide the problem from developers who will
> unknowingly commit broken ebuilds and cause users of alternative package
> managers to suffer.

We've seen in https://bugs.gentoo.org/719202 that developers can already
do that with existing versions of portage, since the failure can be
dependent on USE configuration.

If we want to proactively prevent this issue, then we need to set a hard
limit on the size of A and abort before we generate the manifest.
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Zac Medico]

On 5/26/20 1:43 AM, Alec Warner wrote:
> On Mon, May 25, 2020 at 9:34 PM Zac Medico  > wrote:
> 
> Since variables like A and AA can contain extremely large values which
> may trigger E2BIG errors during attempts to execute subprocesses, delay
> export until the last moment, and unexport when appropriate.
> 
> 
> So I think if you want to do this because PMS says:
>  AA should not be visible in EAPI > 3.
>  A should only be visible in src_*, pkg_nofetch.
> 
> That part of the patch makes sense to me. The part that is confusing to
> me is the 'delay' part; can you explain that further? When you say
> "delay until the last moment" what do you mean by that and what value is
> it delivering?

If we export an environment variable which contains an extremely large
value, then there's a vulnerability in execve which causes it to fail
with an E2BIG error. Since A and AA values can easily grow large enough
to trigger this vulnerability, portage can protect itself from execve
failures by delaying the export until the moment that it hands control
to the ebuild phase.

> Is it simply that we don't export these variables on the python side,
> and we only use them in the shell portion?

That's correct. Here's a test case which demonstrates the E2BIG error,
and shows that 'export -n A' can suppress it:

$ A=$(dd if=/dev/zero bs=1M count=1 | tr '\0' ' ')
10+0 records in
10+0 records out
10485760 bytes (10 MB, 10 MiB) copied, 0.086557 s, 121 MB/s
$ echo ${#A}
10485760
$ export A
$ ls
bash: /bin/ls: Argument list too long
$ export -n A
$ /bin/echo hello world
hello world

-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Alec Warner]

On Mon, May 25, 2020 at 9:34 PM Zac Medico  wrote:

> Since variables like A and AA can contain extremely large values which
> may trigger E2BIG errors during attempts to execute subprocesses, delay
> export until the last moment, and unexport when appropriate.
>

So I think if you want to do this because PMS says:
 AA should not be visible in EAPI > 3.
 A should only be visible in src_*, pkg_nofetch.

That part of the patch makes sense to me. The part that is confusing to me
is the 'delay' part; can you explain that further? When you say "delay
until the last moment" what do you mean by that and what value is it
delivering?

Is it simply that we don't export these variables on the python side, and
we only use them in the shell portion?

-A


> Bug: https://bugs.gentoo.org/720180
> Signed-off-by: Zac Medico 
> ---
>  bin/eapi.sh   |  9 +
>  bin/isolated-functions.sh | 34 
>  bin/phase-functions.sh| 13 +++
>  .../ebuild/_config/special_env_vars.py|  7 +++-
>  lib/portage/package/ebuild/config.py  | 39 ++-
>  5 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/bin/eapi.sh b/bin/eapi.sh
> index 29dfb008c..f56468e4a 100644
> --- a/bin/eapi.sh
> +++ b/bin/eapi.sh
> @@ -26,6 +26,15 @@ ___eapi_has_S_WORKDIR_fallback() {
>
>  # VARIABLES
>
> +___eapi_exports_A() {
> +   # https://bugs.gentoo.org/721088
> +   true
> +}
> +
> +___eapi_exports_AA() {
> +   [[ ${1-${EAPI-0}} =~ ^(0|1|2|3)$ ]]
> +}
> +
>  ___eapi_has_prefix_variables() {
> [[ ! ${1-${EAPI-0}} =~ ^(0|1|2)$ || " ${FEATURES} " == *"
> force-prefix "* ]]
>  }
> diff --git a/bin/isolated-functions.sh b/bin/isolated-functions.sh
> index fde684013..973450d86 100644
> --- a/bin/isolated-functions.sh
> +++ b/bin/isolated-functions.sh
> @@ -107,6 +107,39 @@ __bashpid() {
> sh -c 'echo ${PPID}'
>  }
>
> +# @FUNCTION: ___eapi_vars_export
> +# @DESCRIPTION:
> +# Export variables for the current EAPI. Calls to this function should
> +# be delayed until the last moment, since exporting these variables
> +# may trigger E2BIG errors suring attempts to execute subprocesses.
> +___eapi_vars_export() {
> +   source "${T}/environment.unexported" || die
> +
> +   if ___eapi_exports_A; then
> +   export A
> +   fi
> +
> +   if ___eapi_exports_AA; then
> +   export AA
> +   fi
> +}
> +
> +# @FUNCTION: ___eapi_vars_unexport
> +# @DESCRIPTION:
> +# Unexport variables that were exported for the current EAPI. This
> +# function should be called after an ebuild phase, in order to unexport
> +# variables that may trigger E2BIG errors during attempts to execute
> +# subprocesses.
> +___eapi_vars_unexport() {
> +   if ___eapi_exports_A; then
> +   export -n A
> +   fi
> +
> +   if ___eapi_exports_AA; then
> +   export -n AA
> +   fi
> +}
> +
>  __helpers_die() {
> if ___eapi_helpers_can_die && [[ ${PORTAGE_NONFATAL} != 1 ]]; then
> die "$@"
> @@ -122,6 +155,7 @@ die() {
>
> set +x # tracing only produces useless noise here
> local IFS=$' \t\n'
> +   ___eapi_vars_unexport
>
> if ___eapi_die_can_respect_nonfatal && [[ $1 == -n ]]; then
> shift
> diff --git a/bin/phase-functions.sh b/bin/phase-functions.sh
> index 90e622e75..df2c0d8de 100644
> --- a/bin/phase-functions.sh
> +++ b/bin/phase-functions.sh
> @@ -146,6 +146,7 @@ __filter_readonly_variables() {
> fi
> fi
>
> +   ___eapi_vars_unexport
> "${PORTAGE_PYTHON:-/usr/bin/python}"
> "${PORTAGE_BIN_PATH}"/filter-bash-environment.py "${filtered_vars}" || die
> "filter-bash-environment.py failed"
>  }
>
> @@ -212,6 +213,7 @@ __ebuild_phase() {
>
>  __ebuild_phase_with_hooks() {
> local x phase_name=${1}
> +   ___eapi_vars_export
> for x in {pre_,,post_}${phase_name} ; do
> __ebuild_phase ${x}
> done
> @@ -223,6 +225,7 @@ __dyn_pretend() {
> __vecho ">>> Remove '$PORTAGE_BUILDDIR/.pretended' to
> force pretend."
> return 0
> fi
> +   ___eapi_vars_export
> __ebuild_phase pre_pkg_pretend
> __ebuild_phase pkg_pretend
> >> "$PORTAGE_BUILDDIR/.pretended" || \
> @@ -236,6 +239,7 @@ __dyn_setup() {
> __vecho ">>> Remove '$PORTAGE_BUILDDIR/.setuped' to force
> setup."
> return 0
> fi
> +   ___eapi_vars_export
> __ebuild_phase pre_pkg_setup
> __ebuild_phase pkg_setup
> >> "$PORTAGE_BUILDDIR/.setuped" || \
> @@ -252,6 +256,7 @@ __dyn_unpack() {
> install -m${PORTAGE_WORKDIR_MODE:-0700} -d "${WORKDIR}" ||
> die "Failed to create dir '${WORKDIR}'"
> fi
> cd "${WORKDIR}" || die "Directory change failed: \`cd
> '${WORKDIR}'\`"
> +   ___eapi_vars_export
> __ebuild_phase pre_sr

Re: [gentoo-portage-dev] [PATCH] config.environ: delay export of A and AA (bug 720180)

[Michał Górny]

On Mon, 2020-05-25 at 21:31 -0700, Zac Medico wrote:
> Since variables like A and AA can contain extremely large values which
> may trigger E2BIG errors during attempts to execute subprocesses, delay
> export until the last moment, and unexport when appropriate.
> 

Please don't.  This will only hide the problem from developers who will
unknowingly commit broken ebuilds and cause users of alternative package
managers to suffer.

-- 
Best regards,
Michał Górny



signature.asc
Description: This is a digitally signed message part