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

2018-06-20 Thread 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 
 ---
  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(, "disable-ticketing,");
  
 -if (hasSecure)
 -virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
 +if (hasSecure) {
 +virBufferAddLit(, "x509-dir=");
 +virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
 +virBufferAsprintf(, ",");
>>>
>>> make syntax-check would have told you:
>>>
>>> src/qemu/qemu_command.c:7980:virBufferAsprintf(, ",");
>>> src/qemu/qemu_command.c:8090:virBufferAsprintf(, ",");
>>>
>>> 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(, "rendernode=%s,", 
 graphics->data.spice.rendernode);
 +virBufferAddLit(, "rendernode=");
 +virQEMUBuildBufferEscapeComma(, 
 graphics->data.spice.rendernode);
 +virBufferAsprintf(, ",");
  }
  }
>>>
>>> Reviewed-by: John Ferlan 
>>>
>>> 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

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

2018-06-19 Thread Cole Robinson 
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 
>>> ---
>>>  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(, "disable-ticketing,");
>>>  
>>> -if (hasSecure)
>>> -virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
>>> +if (hasSecure) {
>>> +virBufferAddLit(, "x509-dir=");
>>> +virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
>>> +virBufferAsprintf(, ",");
>>
>> make syntax-check would have told you:
>>
>> src/qemu/qemu_command.c:7980:virBufferAsprintf(, ",");
>> src/qemu/qemu_command.c:8090:virBufferAsprintf(, ",");
>>
>> 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(, "rendernode=%s,", 
>>> graphics->data.spice.rendernode);
>>> +virBufferAddLit(, "rendernode=");
>>> +virQEMUBuildBufferEscapeComma(, 
>>> graphics->data.spice.rendernode);
>>> +virBufferAsprintf(, ",");
>>>  }
>>>  }
>>
>> Reviewed-by: John Ferlan 
>>
>> 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

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-06-19 Thread Cole Robinson 
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 
>> ---
>>  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(, "disable-ticketing,");
>>  
>> -if (hasSecure)
>> -virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
>> +if (hasSecure) {
>> +virBufferAddLit(, "x509-dir=");
>> +virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
>> +virBufferAsprintf(, ",");
> 
> make syntax-check would have told you:
> 
> src/qemu/qemu_command.c:7980:virBufferAsprintf(, ",");
> src/qemu/qemu_command.c:8090:virBufferAsprintf(, ",");
> 
> 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(, "rendernode=%s,", 
>> graphics->data.spice.rendernode);
>> +virBufferAddLit(, "rendernode=");
>> +virQEMUBuildBufferEscapeComma(, 
>> graphics->data.spice.rendernode);
>> +virBufferAsprintf(, ",");
>>  }
>>  }
> 
> Reviewed-by: John Ferlan 
> 
> 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

Thanks,
Cole


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-06-18 Thread John Ferlan 



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 
> ---
>  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(, "disable-ticketing,");
>  
> -if (hasSecure)
> -virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
> +if (hasSecure) {
> +virBufferAddLit(, "x509-dir=");
> +virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
> +virBufferAsprintf(, ",");

make syntax-check would have told you:

src/qemu/qemu_command.c:7980:virBufferAsprintf(, ",");
src/qemu/qemu_command.c:8090:virBufferAsprintf(, ",");

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(, "rendernode=%s,", 
> graphics->data.spice.rendernode);
> +virBufferAddLit(, "rendernode=");
> +virQEMUBuildBufferEscapeComma(, 
> graphics->data.spice.rendernode);
> +virBufferAsprintf(, ",");
>  }
>  }

Reviewed-by: John Ferlan 

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.

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-06-18 Thread Anya Harter 
Add comma escaping for cfg->spiceTLSx509certdir and
graphics->data.spice.rendernode.

Signed-off-by: Anya Harter 
---
 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(, "disable-ticketing,");
 
-if (hasSecure)
-virBufferAsprintf(, "x509-dir=%s,", cfg->spiceTLSx509certdir);
+if (hasSecure) {
+virBufferAddLit(, "x509-dir=");
+virQEMUBuildBufferEscapeComma(, cfg->spiceTLSx509certdir);
+virBufferAsprintf(, ",");
+}
 
 switch (graphics->data.spice.defaultMode) {
 case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
@@ -8082,7 +8085,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr 
cfg,
 goto error;
 }
 
-virBufferAsprintf(, "rendernode=%s,", 
graphics->data.spice.rendernode);
+virBufferAddLit(, "rendernode=");
+virQEMUBuildBufferEscapeComma(, 
graphics->data.spice.rendernode);
+virBufferAsprintf(, ",");
 }
 }
 
diff --git a/tests/qemuxml2argvdata/name-escape.args 
b/tests/qemuxml2argvdata/name-escape.args
index d3b908a7e6..72ed2e8410 100644
--- a/tests/qemuxml2argvdata/name-escape.args
+++ b/tests/qemuxml2argvdata/name-escape.args
@@ -33,6 +33,7 @@ cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
 -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \
 -netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \
 -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \
--spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock \
--vga cirrus \
+-spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\
+rendernode=/dev/dri/foo,,bar \
+-device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/name-escape.xml 
b/tests/qemuxml2argvdata/name-escape.xml
index 9ca7be5968..0580de1813 100644
--- a/tests/qemuxml2argvdata/name-escape.xml
+++ b/tests/qemuxml2argvdata/name-escape.xml
@@ -19,6 +19,7 @@
 
 
   
+  
 
 
   
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 7468537c68..ade21f5a10 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2761,6 +2761,11 @@ mymain(void)
 QEMU_CAPS_DEVICE_CIRRUS_VGA,
 QEMU_CAPS_SPICE,
 QEMU_CAPS_SPICE_UNIX,
+QEMU_CAPS_DEVICE_VIRTIO_GPU,
+QEMU_CAPS_VIRTIO_GPU_VIRGL,
+QEMU_CAPS_SPICE_GL,
+QEMU_CAPS_SPICE_RENDERNODE,
+QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
 QEMU_CAPS_DEVICE_ISA_SERIAL,
 QEMU_CAPS_CHARDEV_FILE_APPEND,
 QEMU_CAPS_CCID_EMULATED);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list