Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sending multiple identical options to a macro will leak them to the next macro accepting the same option #3056

Closed
hroncok opened this issue Apr 23, 2024 · 6 comments · Fixed by #3059
Assignees
Labels
Milestone

Comments

@hroncok
Copy link
Contributor

hroncok commented Apr 23, 2024

Describe the bug
Consider this situation:

Macros:

%macro_leaking(E:) %{nil}
%macro_infected(E:) echo -- "%{-E:-E was provided: %{-E*}}%{!-E:there was no -E}"

Notice both macros take an -E option with a value. The exact name of that option is not limited to E.

And run:

%macro_leaking -E myEoption1 -E myEoption2
%macro_infected
%macro_infected
%macro_infected

Results in:


echo -- "-E was provided: myEoption1"
echo -- "there was no -E"
echo -- "there was no -E"

See that the value passed to -E leaks to the infected macro. Moreover:

%macro_leaking -E myEoption1 -E myEoption2 -E myEoption3 -E myEoption4
%macro_infected
%macro_infected
%macro_infected
%macro_infected

Leads to:


echo -- "-E was provided: myEoption3"
echo -- "-E was provided: myEoption2"
echo -- "-E was provided: myEoption1"
echo -- "there was no -E"

The leaking and infected macros can even be the same:

%macro_infected -E myEoption1 -E myEoption2 -E myEoption3 -E myEoption4
%macro_infected
%macro_infected
%macro_infected
%macro_infected

Leads to:

echo -- "-E was provided: myEoption4"
echo -- "-E was provided: myEoption3"
echo -- "-E was provided: myEoption2"
echo -- "-E was provided: myEoption1"
echo -- "there was no -E"

To Reproduce

Spec:

Name:           reproducer-eee
Version:        0
Release:        0
Summary:        ...
License:        ...

%description
...

%define macro_leaking(E:) %{nil}
%define macro_infected(E:) echo -- "%{-E:-E was provided: %{-E*}}%{!-E:there was no -E}"

%macro_leaking -E myEoption1 -E myEoption2
%macro_infected
%macro_infected
%macro_infected

Run rpmspec -P reproducer-eee.spec.


This impacts macros in Fedora. When I run:

%check
%pyproject_check_import -e '*django*' -e '*flask*' -e '*httpx*' -e '*requests*' -e '*sqla*' -e '*starlette*'
%tox

The %tox macro will receive one of the -e values (coincidentally, %tox also uses -e for one of its options).

Expected behavior
Passing option values multiple times should never leak to other macro calls.

Environment

  • OS / Distribution: Fedora 39, 41
  • Version rpm-4.19.1.1-1.fc39, rpm-4.19.1.1-1.fc40, rpm-4.19.90-0.git16794.1.fc41
@mlschroe
Copy link
Contributor

Yes, it's because the engine pushes the macro with each option but only pops it once.
I guess we could modify freeArgs that it pops until the level is clear, or we could change setupArgs so that it pushes just once.

But see also #2449 that asks of a saner handling of multiple identical option.

@hroncok
Copy link
Contributor Author

hroncok commented Apr 23, 2024

Saner handling of multiple identical options would be nice, but even without that, the described behavior is wrong. Agreed?

@pmatilai
Copy link
Member

pmatilai commented Apr 24, 2024

It's a bug alright. I actually just stumbled on that code a couple of days ago thinking this doesn't look right. It isn't. The bug is as old as the macro engine itself, close to 26 years 😆

It's not just multiple identical options, it's any local macro multiply defined, issue being that freeArgs() only ever pops once. Easily reproduced with eg

rpm --define "aa 0" --define "my() %{define:aa 1} %{define:aa 2}" --eval "%my" --eval "%aa"
warning: Macro %aa defined but not used within scope

1

It should "obviously" output 0.

@pmatilai pmatilai added the bug label Apr 24, 2024
@pmatilai pmatilai added this to RPM Apr 24, 2024
@github-project-automation github-project-automation bot moved this to Backlog in RPM Apr 24, 2024
@pmatilai pmatilai moved this from Backlog to Priority in RPM Apr 24, 2024
@pmatilai pmatilai added this to the 4.20.0 milestone Apr 24, 2024
pmatilai added a commit to pmatilai/rpm that referenced this issue Apr 24, 2024
freeArgs() only popped any local macros once, so if a local macro was
pushed multiple times, whether through %define or multiple identical
options getting passed, we leaked any remaining macros to the outside
scope.

Simply pop the local macros in a loop to fix. Have the internal
popMacro() return the previous pointer (if any) to simplify the job.
We even had an expected-fail test for this, which now passes.

This bug was circa 26 years old. Some might call it vintage at this point.

Fixes: rpm-software-management#3056
@pmatilai pmatilai self-assigned this Apr 24, 2024
@pmatilai pmatilai moved this from Priority to In Review in RPM Apr 24, 2024
pmatilai added a commit to pmatilai/rpm that referenced this issue Apr 24, 2024
freeArgs() only popped any local macros once, so if a local macro was
pushed multiple times, whether through %define or multiple identical
options getting passed, we leaked any remaining macros to the outside
scope.

Simply pop the local macros in a loop to fix. Have the internal
popMacro() return the previous pointer (if any) to simplify the job.
We even had an expected-fail test for this, now passing, but add a
more straightforward reproducer too.

This bug was circa 26 years old. Some might call it vintage at this point.

Fixes: rpm-software-management#3056
ffesti pushed a commit that referenced this issue Apr 24, 2024
freeArgs() only popped any local macros once, so if a local macro was
pushed multiple times, whether through %define or multiple identical
options getting passed, we leaked any remaining macros to the outside
scope.

Simply pop the local macros in a loop to fix. Have the internal
popMacro() return the previous pointer (if any) to simplify the job.
We even had an expected-fail test for this, now passing, but add a
more straightforward reproducer too.

This bug was circa 26 years old. Some might call it vintage at this point.

Fixes: #3056
@github-project-automation github-project-automation bot moved this from In Review to Done in RPM Apr 24, 2024
@pmatilai
Copy link
Member

Oh and thanks for reporting! Like the testcase shows, we kinda knew about this all along but had forgotten, and who knows how long more this might've lurked along 😄

@hroncok
Copy link
Contributor Author

hroncok commented Apr 25, 2024

Is there anything I need to do to initiate a backport to 4.19.x?

@pmatilai
Copy link
Member

No, we'll cherry-pick obvious fixes when doing the next release.

dmnks pushed a commit to dmnks/rpm that referenced this issue May 17, 2024
freeArgs() only popped any local macros once, so if a local macro was
pushed multiple times, whether through %define or multiple identical
options getting passed, we leaked any remaining macros to the outside
scope.

Simply pop the local macros in a loop to fix. Have the internal
popMacro() return the previous pointer (if any) to simplify the job.
We even had an expected-fail test for this, now passing, but add a
more straightforward reproducer too.

This bug was circa 26 years old. Some might call it vintage at this point.

Fixes: rpm-software-management#3056
(cherry picked from commit 695b5c2)
dmnks pushed a commit that referenced this issue May 17, 2024
freeArgs() only popped any local macros once, so if a local macro was
pushed multiple times, whether through %define or multiple identical
options getting passed, we leaked any remaining macros to the outside
scope.

Simply pop the local macros in a loop to fix. Have the internal
popMacro() return the previous pointer (if any) to simplify the job.
We even had an expected-fail test for this, now passing, but add a
more straightforward reproducer too.

This bug was circa 26 years old. Some might call it vintage at this point.

Fixes: #3056
(cherry picked from commit 695b5c2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants