On 12/13/2016 11:43 AM, Erik Skultety wrote: > 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@xxxxxxxxxx> >>>>>>> --- >>>>>>> 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 > The only thing holding up this patch would be your update to tools/virt-admin.pod to describe the name of the command (there was no daemon-log-info). Like I said the escaping was only a question of whether it was necessary - I think you've answered that. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
