Bug 187461 - sysrc(8) mishandles variable names containing a dot
Summary: sysrc(8) mishandles variable names containing a dot
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.0-STABLE
Hardware: Any Any
: Normal Affects Many People
Assignee: Devin Teske
URL:
Keywords:
: 215984 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-03-11 23:10 UTC by Adam McDougall
Modified: 2018-06-20 06:28 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adam McDougall 2014-03-11 23:10:00 UTC
Recently I was trying to convert some "echo a.b.c=1 > /etc/sysctl.conf" type statements to use sysrc instead.  It appears if the variable contains one or more dots, sysrc will write the desired value, fail to read it, and it will always create a new entry if executed again.  This is inconvenient for editing values in /etc/sysctl.conf using the -f parameter because such entries almost always have a dot in the variable.

sysrc -f /etc/sysctl.conf

Symptoms are present on FreeBSD 9 too.

How-To-Repeat: root@build10:~ # grep a.b /etc/rc.conf
root@build10:~ # sysrc a.b=yes
a.b:  -> 
root@build10:~ # grep a.b /etc/rc.conf
a.b="yes"
root@build10:~ # sysrc a.b=yes
a.b:  -> 
root@build10:~ # grep a.b /etc/rc.conf
a.b="yes"
a.b="yes"
Comment 1 Eitan Adler freebsd_committer freebsd_triage 2014-03-12 00:36:24 UTC
Responsible Changed
From-To: freebsd-bugs->dteske

Over to maintainer.
Comment 2 Devin Teske freebsd_committer 2014-03-19 22:28:01 UTC
While I can agree that a tool *like* sysrc is needed for
editing files like /etc/sysctl.conf (and /boot/loader.conf),
sysrc is not that tool.

The tool for that job is being developed and is named
"sysconf". Here is the code which is written in C and is
almost functional (can read but not yet write out changes):

http://druidbsd.cvs.sf.net/viewvc/druidbsd/sysconf/

The sysconf tool will have a syntax that is nearly identical
to sysrc. The break-down between functionality that
requires two separate utilities is as-such:

sysrc is written in shell and uses shell to parse shell (rc-
files are actually shell).

sysconf is written in C and uses C to parse text while
keeping to the rules that are used by loader.4th to
parse loader.conf (which happens to be slightly more
strict but compatible with how sysctl.conf is parsed).

The tool should fit the job.

While the bigger picture is to wait for sysconf, would you
be interested in a patch that actively aborts sysrc if you
attempt to pass a variable that appears invalid?

I'm 50% looking for a POLA violation to say we should
definitely filter input, and 50% saying we shouldn't stop
the user from doing something that appears incorrect
because we could potentially prevent them from doing
something clever.

As it stands, I fully expect the error that comes from
asking sh(1) to expand a variable that contains dots.
-- 
Devin

_____________
The information contained in this message is proprietary and/or confidential. If you are not the intended recipient, please: (i) delete the message and all copies; (ii) do not disclose, distribute or use the message in any manner; and (iii) notify the sender immediately. In addition, please be aware that any message addressed to our domain is subject to archiving and review by persons other than the intended recipient. Thank you.
Comment 3 Adam McDougall 2014-03-20 02:39:57 UTC
On 03/19/2014 18:28, dteske@FreeBSD.org wrote:
> While I can agree that a tool *like* sysrc is needed for
> editing files like /etc/sysctl.conf (and /boot/loader.conf),
> sysrc is not that tool.

Fair enough.  I'll admit I've used it to modify /etc/periodic.conf as
well, but that is probably similar enough.

> 
> The tool for that job is being developed and is named
> "sysconf". Here is the code which is written in C and is
> almost functional (can read but not yet write out changes):
> 
> http://druidbsd.cvs.sf.net/viewvc/druidbsd/sysconf/
> 
> The sysconf tool will have a syntax that is nearly identical
> to sysrc. The break-down between functionality that
> requires two separate utilities is as-such:
> 
> sysrc is written in shell and uses shell to parse shell (rc-
> files are actually shell).
> 
> sysconf is written in C and uses C to parse text while
> keeping to the rules that are used by loader.4th to
> parse loader.conf (which happens to be slightly more
> strict but compatible with how sysctl.conf is parsed).
> 
> The tool should fit the job.

That sounds like it would probably be faster to execute too.

> 
> While the bigger picture is to wait for sysconf, would you
> be interested in a patch that actively aborts sysrc if you
> attempt to pass a variable that appears invalid?
> 
> I'm 50% looking for a POLA violation to say we should
> definitely filter input, and 50% saying we shouldn't stop
> the user from doing something that appears incorrect
> because we could potentially prevent them from doing
> something clever.
> 
> As it stands, I fully expect the error that comes from
> asking sh(1) to expand a variable that contains dots.
> 

I feel torn, part of me is satisfied with the explanation of
"it's meant for shell-compatible variables only" (maybe a
more explicit mention of that in the manpage would help) but
part of me is still bothered that it silently makes a
modification to the specified file while visually indicating
that it did not.  I was probably further drawn into the allure
because sysrc(8) states "This utility works similar to sysctl"
:) sysrc(8) also makes claims of appending only once and
"taking care not to allow the file to grow unwieldy should sysrc
be called repeatedly" which is not holding true in this
circumstance.  Is it possible to withhold the file editing
until sysrc has executed far enough along that it realizes it
thinks it is trying to set a null variable and abort?  Thanks
for your attention.
Comment 4 Eitan Adler freebsd_committer freebsd_triage 2017-12-31 07:58:42 UTC
For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped
Comment 5 Devin Teske freebsd_committer 2018-05-28 19:04:50 UTC
(In reply to ebay from comment #3)

When I went to add the code to block parameters matching invalid variable names, it simply wasn't possible due to documented support for parameter expansion.

From the sysrc(8) manual:

     In addition to the above syntax, sysrc also supports inline sh(1)
     PARAMETER expansion for changing the way values are reported, shown
     below:

     sysrc 'hostname%%.*'
           returns $hostname up to (but not including) first `.'.

     sysrc 'network_interfaces%%[$IFS]*'
           returns first word of $network_interfaces.

     sysrc 'ntpdate_flags##*[$IFS]'
           returns last word of $ntpdate_flags (time server address).

     sysrc usbd_flags-"default"
           returns $usbd_flags or default if unset or NULL.

     sysrc cloned_interfaces+"alternate"
           returns alternate if $cloned_interfaces is set.

I will have to build an exclusion list based on possible shell expansion parameter possibilities to get at the actual variable name for testing.
Comment 6 commit-hook freebsd_committer 2018-05-28 23:34:40 UTC
A commit references this bug:

Author: dteske
Date: Mon May 28 23:34:23 UTC 2018
New revision: 334303
URL: https://svnweb.freebsd.org/changeset/base/334303

Log:
  sysrc(8): Test variable names for invalid characters

  PR:		bin/187461
  Reported by:	ebay@looksharp.net
  MFC after:	4 weeks
  X-MFC-to:	stable/11 (after 11.2-R)
  Sponsored by:	Smule, Inc.

Changes:
  head/usr.sbin/sysrc/sysrc
Comment 7 Devin Teske freebsd_committer 2018-05-28 23:37:28 UTC
Here's what sysrc does now:

$ sysrc ifconfig.ix0=abc
sysrc: ifconfig.ix0: name contains characters not allowed in shell

All examples in the manual were tested to make sure no regressions were introduced. I'll keep this bug open for the next 4 weeks until I merge back to stable/11 (after which I'll close this bug)
Comment 8 Mateusz Piotrowski freebsd_committer 2018-05-30 11:15:42 UTC
This change broke my bootstrap scripts[1] but from what I read in this thread it was unavoidable.

Is there a dedicated tool in the base for setting things like hint.hdaa.1.nid22.config="as=1 seq=15"? Apart from grep and echo of course :)

[1]: https://github.com/0mp/dotfiles/blob/master/freebsd-yoga314/setup.root#L11-L28
Comment 9 Devin Teske freebsd_committer 2018-05-30 17:53:04 UTC
(In reply to Mateusz Piotrowski from comment #8)

Would it help if I made the check a warning instead of fatal?
Comment 10 Mateusz Piotrowski freebsd_committer 2018-05-30 23:54:44 UTC
(In reply to Devin Teske from comment #9)

I am not sure if this is a good idea. Unless sysrc is officially supporting a.b.c.d-like variables I am not going to use it in such situations unfortunately. This undefined behavior might be broken anytime.

Previously, I thought that sysrc is the tool to edit all those somewhat shell-looking configuration files on FreeBSD. Now that I know it's not I'll have to look for a substitute.

Actually, maybe we could add a flag which turns on support for a.b.c.d-like variables? Or even more generally: a flag which adds additional characters to VALID_VARNAME_CHARS.


(Unless we're migrating to UCL with all those config files soon... :) )

What do you think?
Comment 11 Devin Teske freebsd_committer 2018-05-31 03:57:16 UTC
(In reply to Mateusz Piotrowski from comment #10)

sysrc doesn't just edit files that look like shell, but files that are in-fact shell. This is to explain that if a file has:

foo=abc
bar=$foo

sysrc knows that at the time the operating system boots, bar is in-fact abc, and if queried ("sysrc bar") displays "foo: abc".

It was important for sysrc to embrace the fact that /etc/rc.conf and friends are shell rather than ignore it and hope nobody writes any shell code in them.

This invariably, as you have noticed, makes sysrc a poor choice for editing any file that either is not in-fact shell or fails to adhere to shell restrictions (e.g., /etc/sysctl.conf).

sysrc is written in shell and it operates on shell.

By that reasoning, the right tool for sysctl.conf and friends should be written in C.

I personally am not a fan of trying to parse every file with libucl nor am I a fan of converting existing configuration file formats to appease a parser. I am also not a fan of complex parsers such as yacc/bison.

My solution to the problem is to leverage existing work that I already have in base:

https://svnweb.freebsd.org/base/head/lib/libfigpar/

That's the first half of two requirements to solve the problem in C with BSD-licensed code. figpar above is the generalized configuration parser (man page below):

https://www.freebsd.org/cgi/man.cgi?query=figpar

I have already incorporated figpar into a tool called "sysconf" which can query values. The missing piece of the puzzle, the other half required, is me to write libfigput as the library to supply the generalized configuration writer for rewriting an existing config with new values.

I prefer this approach because it is super light-weight and super portable. The figpar code was actually written by me on Debian Linux 2.2 (code named "Potato") and I ported it to FreeBSD after I became a FreeBSD developer; but I also had access to the [long-since shutdown] SourceForge compile farm back then and so I know figpar works on every platform I could get my hands on. That's the same aim that I have with figput and sysconf.
Comment 12 Devin Teske freebsd_committer 2018-06-17 03:14:59 UTC
If OK, I am going to merge 334303 to stable/11, after which I will close this bug
Comment 13 Mateusz Piotrowski freebsd_committer 2018-06-19 14:54:21 UTC
(In reply to Devin Teske from comment #12)

+1
Comment 14 commit-hook freebsd_committer 2018-06-20 06:24:25 UTC
A commit references this bug:

Author: dteske
Date: Wed Jun 20 06:24:03 UTC 2018
New revision: 335409
URL: https://svnweb.freebsd.org/changeset/base/335409

Log:
  MFC r334303: sysrc(8): Test variable names for invalid characters

  PR:		bin/187461
  Reported by:	ebay@looksharp.net
  Sponsored by:	Smule, Inc.

Changes:
_U  stable/11/
  stable/11/usr.sbin/sysrc/sysrc
Comment 15 Devin Teske freebsd_committer 2018-06-20 06:25:55 UTC
Thank you so very much for your extended patience in awaiting resolution.
Comment 16 Devin Teske freebsd_committer 2018-06-20 06:28:02 UTC
*** Bug 215984 has been marked as a duplicate of this bug. ***