udev-rules: fix matching of token types that support alternative patterns by ldv-alt · Pull Request #26886 · systemd/systemd

ldv-alt · 2023-03-19T00:14:41Z

For those token types that support matching of alternative patterns,
their token values are interpreted as nulstr, so make sure the parser
does the right thing and makes these token values terminated by two
subsequent NULs so they could be safely interpreted as nulstr.

Before this fix, the following rules would result to "echo foo" invocation:

  ENV{foo}=", RUN"
  ENV{foo}=="bar", RUN+="echo foo"

because the value of ENV{foo} is treated as nulstr, and it used to match
against alternative patterns, in this case bar, , RUN, and ="echo foo.

yuwata · 2023-03-19T05:27:53Z

Before this fix, the following rules would result to "echo foo" invocation:

ENV{foo}=", RUN"
ENV{foo}=="bar", RUN+="echo foo"

How the example was parsed previously?
Could you extend the commit message?

ldv-alt · 2023-03-19T08:24:00Z

Before this fix, the following rules would result to "echo foo" invocation:
ENV{foo}=", RUN"
ENV{foo}=="bar", RUN+="echo foo"

How the example was parsed previously? Could you extend the commit message?

The value of ENV{foo} is treated as nulstr, and it was matching against alternative patterns, in this case bar, , RUN, and ="echo foo.

yuwata · 2023-03-19T11:14:45Z

Before this fix, the following rules would result to "echo foo" invocation:
ENV{foo}=", RUN"
ENV{foo}=="bar", RUN+="echo foo"

How the example was parsed previously? Could you extend the commit message?

The value of ENV{foo} is treated as nulstr, and it was matching against alternative patterns, in this case bar, , RUN, and ="echo foo.

Oh,,, terrible...

yuwata

LGTM. But please extend the commit message of the first commit.

…erns For those token types that support matching of alternative patterns, their token values are interpreted as nulstr, so make sure the parser does the right thing and makes these token values terminated by two subsequent NULs so they could be safely interpreted as nulstr. Before this fix, the following rules would result to "echo foo" invocation: ENV{foo}=", RUN" ENV{foo}=="bar", RUN+="echo foo" because the value of `ENV{foo}` is treated as nulstr, and it used to match against alternative patterns, in this case `bar`, `, RUN`, and `="echo foo`. Fixes: 25de7aa ("udev: modernize udev-rules.c")

This reverts commit cd3c8a1 which was papering over the bug instead of a proper fix made by the previous commit.

Fix check for conflicting and duplicate expressions of types that support alternative patterns. Fixes: 3ec58d0 ("udev-rules: check for conflicting and duplicate expressions")

ldv-alt · 2023-03-19T12:00:55Z

LGTM. But please extend the commit message of the first commit.

Done, please take a look.

yuwata · 2023-03-19T12:12:02Z

From the commit message:

in this case bar, , RUN, and +="echo foo.

Maybe, "+" is mistakenly inserted?

ldv-alt · 2023-03-19T12:14:31Z

btw, this might obsolete commit cd3c8a1 which seems to paper over this bug.

yuwata · 2023-03-19T12:16:30Z

btw, this might obsolete commit cd3c8a1 which seems to paper over this bug.

Indeed. Yeah, I agree. Feel free to revert it.

ldv-alt · 2023-03-19T12:45:16Z

From the commit message:

in this case bar, , RUN, and +="echo foo.

Maybe, "+" is mistakenly inserted?

It was a typo, fixed, thanks!

ldv-alt · 2023-03-19T12:46:11Z

btw, this might obsolete commit cd3c8a1 which seems to paper over this bug.

Indeed. Yeah, I agree. Feel free to revert it.

Added a revert commit to this series.

yuwata

LGTM. Thank you!

keszybz · 2023-03-28T16:21:18Z

The first two commits backported to v253-stable. I assume the last one is not needed.

mbiebl · 2023-07-15T05:06:24Z

This seems to have caused a regression, see
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1040956

@ldv-alt can you please take a look?

yuwata · 2023-07-15T05:51:13Z

@mbiebl Filed as #28411. Please open a new issue instead of commenting on an already merged PR, otherwise we can easily forget the comment.

mbiebl · 2023-07-15T06:12:30Z

Thanks @yuwata .
Wasn't entirely sure yet, whether it is really a regression or a deliberate change in behaviour which is why I wanted to ask first.

yuwata · 2023-07-15T06:23:58Z

Even if you are not sure, opening issue is fine. Especially, when the issue is in such a core part of systemd.

github-actions bot added tests udev util-lib please-review PR is ready for (re-)review by a maintainer labels Mar 19, 2023

This comment was marked as off-topic.

Sign in to view

ldv-alt changed the title ~~udev-rules: fix matching of token types that support alternative pattern~~ udev-rules: fix matching of token types that support alternative patterns Mar 19, 2023

yuwata approved these changes Mar 19, 2023

View reviewed changes

yuwata added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Mar 19, 2023

ldv-alt added 3 commits March 19, 2023 11:32

Revert "udev: prepare memory for extra NUL termination for NULSTR"

8c499a6

This reverts commit cd3c8a1 which was papering over the bug instead of a proper fix made by the previous commit.

udev-rules: fix check for conflicting and duplicate expressions

5004aa8

Fix check for conflicting and duplicate expressions of types that support alternative patterns. Fixes: 3ec58d0 ("udev-rules: check for conflicting and duplicate expressions")

yuwata added the needs-stable-backport label Mar 19, 2023

ldv-alt force-pushed the udevadm-verify branch from 9067973 to 7f60c14 Compare March 19, 2023 11:58

ldv-alt force-pushed the udevadm-verify branch from 7f60c14 to 5004aa8 Compare March 19, 2023 12:41

yuwata approved these changes Mar 19, 2023

View reviewed changes

yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Mar 19, 2023

yuwata merged commit a9b402e into systemd:main Mar 20, 2023

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Mar 20, 2023

keszybz removed the needs-stable-backport label Mar 28, 2023

yuwata mentioned this pull request Jul 15, 2023

udev: regression? #28411

Closed

Uh oh!

Conversation

ldv-alt commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

yuwata commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldv-alt commented Mar 19, 2023

Uh oh!

yuwata commented Mar 19, 2023

Uh oh!

yuwata left a comment

Choose a reason for hiding this comment

Uh oh!

ldv-alt commented Mar 19, 2023

Uh oh!

yuwata commented Mar 19, 2023

Uh oh!

ldv-alt commented Mar 19, 2023

Uh oh!

yuwata commented Mar 19, 2023

Uh oh!

ldv-alt commented Mar 19, 2023

Uh oh!

ldv-alt commented Mar 19, 2023

Uh oh!

yuwata left a comment

Choose a reason for hiding this comment

Uh oh!

keszybz commented Mar 28, 2023

Uh oh!

mbiebl commented Jul 15, 2023

Uh oh!

yuwata commented Jul 15, 2023

Uh oh!

mbiebl commented Jul 15, 2023

Uh oh!

yuwata commented Jul 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ldv-alt commented Mar 19, 2023 •

edited

Loading

yuwata commented Mar 19, 2023 •

edited

Loading