|Summary:||pmconfig -L output quoting insufficient|
|Product:||[Fedora] Fedora||Reporter:||Frank Ch. Eigler <fche>|
|Component:||pcp||Assignee:||Nathan Scott <nathans>|
|Status:||CLOSED ERRATA||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Version:||rawhide||CC:||brolley, fche, mgoodwin, nathans, pcp, scox|
|Fixed In Version:||pcp-3.9.2-1.el5||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2014-04-16 16:27:16 UTC||Type:||Bug|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Cloudforms Team:||---||Target Upstream Version:|
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 "188.8.131.52" _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.