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

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.

> > 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

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.