Re: [libvirt] [PATCH v2 08/10] virt-admin: Wire-up the logging APIs

On Tue, Dec 13, 2016 at 09:50:49AM -0500, John Ferlan wrote: > > > On 12/13/2016 09:32 AM, Erik Skultety wrote: >> On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote: >>> >>> >>> On 12/12/2016 10:42 AM, Erik Skultety wrote: >>>> On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote: >>>>> >>>>> >>>>> On 11/25/2016 08:12 AM, Erik Skultety wrote: >>>>>> Finally, now that all APIs have been introduced, wire them up to virt-admin >>>>>> and introduce daemon-log-outputs and daemon-log-filters commands. >>>>>> >>>>>> Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; >>>>>> --- >>>>>> tools/virt-admin.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> tools/virt-admin.pod | 90 ++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 210 insertions(+) >>>>>> >>>>> >>>>> Now it *is* the coffee - should there be any concerns along the way over >>>>> escaping characters? I think this is the only place where we >>>>> read/print, but via the API if someone passes a string - do we need to >>>>> worry about that? Yes, I know - not the normal thing, but you know >>>>> someone will try... >>>> >>>> Is it an issue though? If someone sends a binary garbage for filename, we have >>>> to create a logfile with that name on the filesystem. When we retrieve it, I >>>> think we should print it as we created it - garbage. The other option we have >>>> is to strip all the control characters and print the rest which in the worst >>>> case scenario will end up as an empty string... >>>> >>> >>> If someone provides garbage I thought our general practice was to escape >>> the 'unprintable' chars when we Format the output. >> >> In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString) >> so that the resulting XML is compliant with the specification. We also do it >> for shell-related stuff like escaping the arguments that will end up on qemu >> cmdline. But what's the purpose of escaping in this case? Additionally, if you >> escape the output and let's say the user saved the output into a environment >> variable, he now has an escaped garbage which is further unusable when instead >> we should have gone with the verbatim, unprintable garbage. >> I played around with this in the morning and found out that bash allows you to >> use structure like this one: $'nnn' (where nnn is the string to be escaped), >> now that would be doable from our perspective, but then, I'm not sure whether >> such a construction is supported with other shells out there. >> > > Escaping wasn't a requirement, just a concern. Hence the original > question "should there be any concerns along the way over > escaping characters? " > > And by escaping I meant only the name of the file, although perhaps that > wasn't clear as I re-read the original comment. No, it was perfectly clear that you meant just the filename, but it doesn't contradict my point anyway. > >>> >>> The first thing that came to mind that might be close to what's being >>> done here was domain interface script, where when we print out the name >>> we Escape whatever is in script path. >>> >>> Looking at the src/util/virtconf.c for VIR_CONF_STRING and >>> virConfParseString would seem to imply to me the ability to read escaped >>> characters. >>> >>> The difference here is that you're now formatting the name; whereas, >>> previously the formatting was entirely left to whomever edited >>> /etc/libvirt/libvirtd.conf. >>> >> >> What difference? I put some garbage into the config via vim's controlling > > semantics... where 'difference' means prior to your changes someone > would have to either edit or append libvirtd.conf in order to > alter/define the name. With these changes the name (while running) is > altered to some file which if it has unprintable characters in the name, > how would it look? > Ugly. Still, they can put a name containing unprintable characters to the config and the daemon will create the file on the filesystem. After my changes, the same logic applies. IOW the user is responsible for the formatting of the filename in both cases. >> sequences and when I used 'cat' on the config, what I got was an unescaped >> garbage, not an escaped version of what I put there, so in that aspect the >> behaviour is the same, user provides us with garbage, and in turn, he gets the >> same garbage back, there shouldn't be any compatibility or functioning issues >> connected to that. >> >>> In the long run, recent memory says whenever there's a user provided >>> character string - the review comments have always been be sure to >>> Escape it. >>> >>> Searching for Escape in tools/*.c found a couple more "output" files. >> >> I looked at those and as I said, those are XML escaping routines and shell >> escaping routines present mainly for reasons I mentioned above. >> > > Fair enough - it's been considered... I think what I was going at there > though was how virsh-snapshot handles output file names where the > 'file=' or 'snapshot=' output is Escaped. > I understand, XMLs are special in this way and since since commit @aeb5262e we strip the control characters completely anyway so even our existing 'escaping' routine for XMLs isn't a proper one. Soo, did we reach a consensus? Erik