[libvirt] [PATCH 4/4] qemu: Escape commas for qemuBuildGrapicsSPICECommandLine

[Anya Harter]

On 06/19/2018 10:46 AM, Cole Robinson wrote:
> On 06/19/2018 09:47 AM, Cole Robinson wrote:
>> On 06/18/2018 07:52 PM, John Ferlan wrote:
>>>
>>>
>>> On 06/18/2018 01:57 PM, Anya Harter wrote:
>>>> Add comma escaping for cfg->spiceTLSx509certdir and
>>>> graphics->data.spice.rendernode.
>>>>
>>>> Signed-off-by: Anya Harter <aharter at redhat.com>
>>>> ---
>>>>  src/qemu/qemu_command.c                 | 11 ++++++++---
>>>>  tests/qemuxml2argvdata/name-escape.args |  5 +++--
>>>>  tests/qemuxml2argvdata/name-escape.xml  |  1 +
>>>>  tests/qemuxml2argvtest.c                |  5 +++++
>>>>  4 files changed, 17 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>>> index a9a5e200e9..36dccea9b2 100644
>>>> --- a/src/qemu/qemu_command.c
>>>> +++ b/src/qemu/qemu_command.c
>>>> @@ -7974,8 +7974,11 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>>>>          !cfg->spicePassword)
>>>>          virBufferAddLit(&opt, "disable-ticketing,");
>>>>  
>>>> -    if (hasSecure)
>>>> -        virBufferAsprintf(&opt, "x509-dir=%s,", cfg->spiceTLSx509certdir);
>>>> +    if (hasSecure) {
>>>> +        virBufferAddLit(&opt, "x509-dir=");
>>>> +        virQEMUBuildBufferEscapeComma(&opt, cfg->spiceTLSx509certdir);
>>>> +        virBufferAsprintf(&opt, ",");
>>>
>>> make syntax-check would have told you:
>>>
>>> src/qemu/qemu_command.c:7980:        virBufferAsprintf(&opt, ",");
>>> src/qemu/qemu_command.c:8090:            virBufferAsprintf(&opt, ",");
>>>
>>> So here and below, I changed to virBufferAddLit before pushing.
>>>
>>>> +    }
>>>>  
>>>>      switch (graphics->data.spice.defaultMode) {
>>>>      case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
>>>> @@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>>>>                  goto error;
>>>>              }
>>>>  
>>>> -            virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode);
>>>> +            virBufferAddLit(&opt, "rendernode=");
>>>> +            virQEMUBuildBufferEscapeComma(&opt, graphics->data.spice.rendernode);
>>>> +            virBufferAsprintf(&opt, ",");
>>>>          }
>>>>      }
>>>
>>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>>
>>> John
>>>
>>> >From the last time I passed through this when Sukrit posted patches,
>>> still to do are qemuBuildHostNetStr, qemuBuildDiskThrottling (for group
>>> name), and various qemuBuildNetworkDriveURI, qemuBuildNetworkDriveStr,
>>> and qemuGetDriveSourceString.
>>>
>>
>> Oh cool, I didn't realize you had found more examples! I looked at some
>> of these with Anya before the patch series.
>>
>> NetworkDriveURI is a private subset of NetworkDriveStr, so the former
>> doesn't need any direct changes AFAICT.
>>
>> qemuGetDriveSourceString is called outside qemu_command.c, for example
>> passing the result to qemu monitor commands. Anyone know if comma
>> escaping applies there too? Same with qemuBuildHostNetStr
>>
> 
> From what I can tell qemuGetDriveSourceString and qemuBuildHostNetStr
> usages outside of qemu_command.c should _not_ have comma escaping, which
> makes sense as the comma isn't used as a delimiter in those substrings.
> So the comma escaping should be done at the call sites of those
> functions in qemu_command.c

By my estimation the BiteSizedTask entry
https://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_more_command_line_values
is complete.

The four bullet points have been addressed as follows:

* Building the source drive string in qemuBuildNetworkDriveURI,
qemuBuildNetworkDriveStr, and qemuGetDriveSourceString
(src->hosts->socket, NBD exportname=src->path, Sheepdog src->path
processing, rbd src->path & configFile processing)

Since some of these functions are called from outside the building of
the command line, escaping within the functions could lead to problems
elsewhere. Thus, the approach is to escape the output of the functions
from their callers.
All calls to qemuBuildNetworkDriveURI are within qemuBuildNetworkDriveStr.
One call to qemuBuildNetworkDriveStr is within qemuGetDriveSourceString,
and the other is within qemuBuildSCSIiSCSIHostdevDrvStr.
The only call to qemuGetDriveSourceString is within
qemuBuildDriveSourceStr and the output is escaped later within this
function. This escaping handles all but the one call to
qemuBuildNetworkDriveStr.
This case is handled in this patch:
https://www.redhat.com/archives/libvir-list/2018-June/msg01462.html

* Validate whether TYPE_FILE needs to follow TYPE_DEV and TYPE_PIPE
handling in qemuBuildChrChardevStr

Addressed in this email:
https://www.redhat.com/archives/libvir-list/2018-June/msg01384.html

* socket.address processing in qemuBuildHostNetStr (localaddr for UDP too)

Cole believes that these entities cannot have commas in them anyways, so
escaping any commas would just delay the inevitable

* group_name processing in qemuBuildDiskThrottling

Handled in this patch:
https://www.redhat.com/archives/libvir-list/2018-June/msg01409.html


Please let me know if there is any more work to be done on this task or
if it is ready to be deleted from the page.

Thanks so much,

Anya