Bug 1085401 - pmconfig -L output quoting insufficient
Summary: pmconfig -L output quoting insufficient
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: pcp
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nathan Scott
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-04-08 14:05 UTC by Frank Ch. Eigler
Modified: 2014-05-01 18:28 UTC (History)
6 users (show)

Fixed In Version: pcp-3.9.2-1.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-04-16 16:27:16 UTC


Attachments (Terms of Use)

Description Frank Ch. Eigler 2014-04-08 14:05:27 UTC
On rawhide today, "pmconfig -L | grep nss" returns the string

nss_version=3.16 Basic ECC

And this is not acceptable to all the uses of   
   eval 'pmconfig -L ...'
throughout the testsuite ("Basic: command not found").

Looking at the insides of pmconfig.c, there is no awareness of strings
possibly containing spaces or special characters (like " or \n).  The
-l and -s options aren't documented, and aren't implemented enough to
quote general strings anyhow.

It looks like we have a total of two users of pmconfig in the source
tree.  Both appear to want bourne-shell-compatible quoting on the
output (-s for mingw, -L for pcpqa), but don't get it.  We should
probably unconditionally (and more carefully) quote the output strings.

Comment 1 Nathan Scott 2014-04-08 23:27:11 UTC
Oh wow, unexpected!  I've not come across an nss_version thats not just the dotted-number variety before.  But, on inspection of the headers, yes they have always had this possibility it seems...

/usr/include/nss3/nss.h:#define NSS_VERSION  "3.13.6.0" _NSS_ECC_STRING _NSS_CUSTOMIZED

... we've just not come across either of those last two macros expand in any shipped product we've seen before until now.

| It looks like we have a total of two users of pmconfig in the source
| tree.  Both appear to want bourne-shell-compatible quoting on the
| output (-s for mingw, -L for pcpqa), but don't get it.  We should

Its primarily used in QA, where there are some 35 tests using it & one of the common.* files (used by 3/4 more tests IIRC).

This is more complicated to fix than it at first seems, as pmconfig -s mode will add the quoting in some situations (though, not sure its always doing what I thought it would do) in which case, we'll get double quoting and breakage will probably result.  Will dig a little deeper, bit of a rats nest.

cheers.

Comment 2 Frank Ch. Eigler 2014-04-09 12:46:49 UTC
The new pmconfig.c code in commit 7c3f89afe22 improves this greatly.
It should probably go a little further in quoting or precluding
questionable strings, such as those that contain " or \n characters.
(For these  printf("%s=\"%s\"", key, value);  is not enough.)

Comment 3 Nathan Scott 2014-04-10 22:14:24 UTC
*nod* - will do - thanks for reviewing.  The back-tick character also needs quoting I suppose, possibly the dollar sign, round/curly braces.  Any others?

We will have successfully shot ourselves in the foot if any of those ever make it from pcp.conf or hard-coded-in-libpcp out to this point, but certainly there's never harm in extra defence against the dark arts.

thanks.

Comment 4 Frank Ch. Eigler 2014-04-11 13:16:45 UTC
The new logic from commit 2ad34572 is even better, yup, though
I believe it misses \ itself.  Perhaps instead of hunting for a
complete blacklist of problematic characters, consider the
whitelist approach, as embodied in pmmgr.cxx sh_quote().

Comment 5 Nathan Scott 2014-04-14 01:12:23 UTC
Excellent point re '\' - re-reviewing the pmconfig change, it seems it is quoting far too much, in fact.  There appears to be different expansion rules applied in the shell depending on whether it is expanding a ""-enclosed string to everything else.  As such, the pmmgr.cxx code also looks like it will incorrectly quote strings as well.  For example:

nathans@smash:~$ echo "(sdf"
(sdf
nathans@smash:~$ echo "\(sdf"
\(sdf

This would be an inappropriate transformation for a string value, right?  (cos that back-quote will now be passed into the called program, instead of shell-expanded, in the case of pmmgr.cxx sh_quote).  Gah, its never easy!  :)

[ this appears to be an issue in pmmgr handling of string constants vs other shell-expanded stuff?  hopefully, I'm overlooking some other magic its doing ]

From further experimentation, the list of characters needing quoting in the pmconfig string constant case is actually very small (smaller than I originally thought, although \ and ! were missed - none of the braces need quoting, as long as dollar-sign is correctly escaped).

Fascinating stuff though, I'm learning, I'm learning - thanks for following up!

Comment 6 Nathan Scott 2014-04-14 01:32:37 UTC
re #c5 - Frank points out on IRC that its all good in pmmgr, as it cleverly is quoting the string start/end too, thus there is no string-constant case like in pmconfig - so all should be well in pmmgr.

Comment 7 Fedora Update System 2014-04-15 20:11:39 UTC
pcp-3.9.2-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pcp-3.9.2-1.el6

Comment 8 Fedora Update System 2014-04-15 20:20:35 UTC
pcp-3.9.2-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/pcp-3.9.2-1.el5

Comment 9 Fedora Update System 2014-04-15 20:25:04 UTC
pcp-3.9.2-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/pcp-3.9.2-1.fc20

Comment 10 Fedora Update System 2014-04-15 20:26:34 UTC
pcp-3.9.2-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/pcp-3.9.2-1.fc19

Comment 11 Fedora Update System 2014-04-16 16:27:02 UTC
Package pcp-3.9.2-1.el5:
* should fix your issue,
* was pushed to the Fedora EPEL 5 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=epel-testing pcp-3.9.2-1.el5'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-EPEL-2014-1151/pcp-3.9.2-1.el5
then log in and leave karma (feedback).

Comment 12 Fedora Update System 2014-04-16 16:27:16 UTC
pcp-3.9.2-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 Fedora Update System 2014-04-17 06:02:22 UTC
pcp-3.9.2-1.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2014-04-25 04:29:07 UTC
pcp-3.9.2-1.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2014-05-01 18:28:24 UTC
pcp-3.9.2-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.