netdev - Re: [PATCH iproute2-next 3/3] lib: utils: Add parse_one_of_deprecated(), parse_on_off

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 15 Nov 2023 17:57:27 +0100
From: Petr Machata <petrm@...dia.com>
To: Stephen Hemminger <stephen@...workplumber.org>
CC: Petr Machata <petrm@...dia.com>, David Ahern <dsahern@...il.com>,
	<netdev@...r.kernel.org>, Patrisious Haddad <phaddad@...dia.com>
Subject: Re: [PATCH iproute2-next 3/3] lib: utils: Add
 parse_one_of_deprecated(), parse_on_off_deprecated()

Stephen Hemminger <stephen@...workplumber.org> writes:

> On Wed, 15 Nov 2023 16:31:59 +0100
> Petr Machata <petrm@...dia.com> wrote:
>
>> The functions parse_on_off() and parse_one_of() currently use matches() for
>> string comparison under the hood. This has some odd consequences. In
>> particular, "o" can be used as a shorthand for "off", which is not obvious,
>> because "o" is the prefix of both. By sheer luck, the end result actually
>> makes some sense: "on" means on, anything else means off (or errors out).
>> Similar issues are in principle also possible for parse_one_of() uses,
>> though currently this does not come up.
>
> This was probably a bug, I am open to breaking shorthand usage in this case.

There were uses of matches() for on/off parsing even before adding
parse_on_off(). The bug was converting _everyone_ to matches().

I figured you'd be against just s/matches/strcmp, but if you think it's
OK, I have no problem with that. Shorthanding on/off just makes no
sense to me, not even by mistake.

How about the parse_one_of() users? E.g. the disabled/check/strict for
macsec validate, I could see someone mistyping it as "disable", so now
it lives in some deployment script, or testing harness, or whatever.

Maybe do the warning thing in this case? And retire it a couple releases
down the line in favor of just accepting strcmp?