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. >> >> 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? > 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. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
