11:05 a.m.
For some reason, we allow a bridge name with %d in it, which we replace
with an unsigned integer to form a bridge name that does not yet exist
on the host.
Do not blindly pass it to virAsprintf if it's not the only conversion,
to prevent crashing on input like:
<network>
<name>test</name>
<forward mode='none'/>
<bridge name='virbr%d%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s'/>
</network>
Ignore any template strings that do not have exactly one %d conversion,
like we do in various drivers before calling virNetDevTapCreateInBridgePort.
---
v2: drop the unnecessary changes in networkBridgeNameValidate
src/network/bridge_driver.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3b879cd..fe2448d 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2775,7 +2775,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
int ret = -1, id = 0;
char *newname = NULL;
- const char *templ = def->bridge ? def->bridge : "virbr%d";
+ const char *templ = "virbr%d";
+ const char *p;
+
+ if (def->bridge &&
+ (p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%')
&&
+ strstr(def->bridge, "%d"))
+ templ = def->bridge;
do {
if (virAsprintf(&newname, templ, id) < 0)
--
2.0.5
11:14 a.m.
On 05/05/2015 10:05 AM, Ján Tomko wrote:
For some reason, we allow a bridge name with %d in it, which we
replace
with an unsigned integer to form a bridge name that does not yet exist
on the host.
Do not blindly pass it to virAsprintf if it's not the only conversion,
to prevent crashing on input like:
<network>
<name>test</name>
<forward mode='none'/>
<bridge name='virbr%d%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s'/>
</network>
Ignore any template strings that do not have exactly one %d conversion,
like we do in various drivers before calling virNetDevTapCreateInBridgePort.
---
v2: drop the unnecessary changes in networkBridgeNameValidate
src/network/bridge_driver.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3b879cd..fe2448d 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2775,7 +2775,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
int ret = -1, id = 0;
char *newname = NULL;
- const char *templ = def->bridge ? def->bridge : "virbr%d";
+ const char *templ = "virbr%d";
+ const char *p;
Unused variable.
+
+ if (def->bridge &&
+ (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
+ strstr(def->bridge, "%d"))
Simpler as:
if (def->bridge &&
strstr(def->bridge, "%d") == strrchr(def->bridge, '%'))
ACK with that simplification.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:26 a.m.
On Tue, May 05, 2015 at 10:14:18AM -0600, Eric Blake wrote:
On 05/05/2015 10:05 AM, Ján Tomko wrote:
> For some reason, we allow a bridge name with %d in it, which we replace
> with an unsigned integer to form a bridge name that does not yet exist
> on the host.
>
> Do not blindly pass it to virAsprintf if it's not the only conversion,
> to prevent crashing on input like:
>
> <network>
> <name>test</name>
> <forward mode='none'/>
> <bridge name='virbr%d%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s'/>
> </network>
>
> Ignore any template strings that do not have exactly one %d conversion,
> like we do in various drivers before calling virNetDevTapCreateInBridgePort.
> ---
> v2: drop the unnecessary changes in networkBridgeNameValidate
>
> src/network/bridge_driver.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3b879cd..fe2448d 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2775,7 +2775,13 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
>
> int ret = -1, id = 0;
> char *newname = NULL;
> - const char *templ = def->bridge ? def->bridge : "virbr%d";
> + const char *templ = "virbr%d";
> + const char *p;
Unused variable.
> +
> + if (def->bridge &&
> + (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
> + strstr(def->bridge, "%d"))
Simpler as:
if (def->bridge &&
strstr(def->bridge, "%d") == strrchr(def->bridge, '%'))
I still don't see it.
[A] strchr(def->bridge, '%')
[B] strrchr(def->bridge, '%')
[C] strstr(def->bridge, "%d"))
When def->bridge is '%s%s%s%d', [A] points to the first %s, [B] points
to the %d and so does [C]
This string would pass the simplified condition (B == C), but not the
full one (A != C)
Jan
ACK with that simplification.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:39 a.m.
On 05/05/2015 10:26 AM, Ján Tomko wrote:
On Tue, May 05, 2015 at 10:14:18AM -0600, Eric Blake wrote:
> On 05/05/2015 10:05 AM, Ján Tomko wrote:
>> For some reason, we allow a bridge name with %d in it, which we replace
>> with an unsigned integer to form a bridge name that does not yet exist
>> on the host.
>>
>> + if (def->bridge &&
>> + (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
>> + strstr(def->bridge, "%d"))
>
> Simpler as:
>
> if (def->bridge &&
> strstr(def->bridge, "%d") == strrchr(def->bridge, '%'))
I still don't see it.
[A] strchr(def->bridge, '%')
[B] strrchr(def->bridge, '%')
[C] strstr(def->bridge, "%d"))
When def->bridge is '%s%s%s%d', [A] points to the first %s, [B] points
to the %d and so does [C]
This string would pass the simplified condition (B == C), but not the
full one (A != C)
Okay, I see your counterargument. Still, strstr() is pretty expensive
compared to just:
if (def->bridge &&
(p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%')
&&
p[1] == 'd')
Jan
>
> ACK with that simplification.
at which point p is no longer a dead variable, but I've still reduced
the code complexity.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:39 a.m.
On 05/05/2015 12:39 PM, Eric Blake wrote:
On 05/05/2015 10:26 AM, Ján Tomko wrote:
> On Tue, May 05, 2015 at 10:14:18AM -0600, Eric Blake wrote:
>> On 05/05/2015 10:05 AM, Ján Tomko wrote:
>>> For some reason, we allow a bridge name with %d in it, which we replace
>>> with an unsigned integer to form a bridge name that does not yet exist
>>> on the host.
>>>
>>> + if (def->bridge &&
>>> + (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
>>> + strstr(def->bridge, "%d"))
>>
>> Simpler as:
>>
>> if (def->bridge &&
>> strstr(def->bridge, "%d") == strrchr(def->bridge,
'%'))
>
> I still don't see it.
>
> [A] strchr(def->bridge, '%')
> [B] strrchr(def->bridge, '%')
> [C] strstr(def->bridge, "%d"))
>
> When def->bridge is '%s%s%s%d', [A] points to the first %s, [B] points
> to the %d and so does [C]
>
> This string would pass the simplified condition (B == C), but not the
> full one (A != C)
Okay, I see your counterargument. Still, strstr() is pretty expensive
compared to just:
if (def->bridge &&
(p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%')
&&
p[1] == 'd')
Coverity complains :
Event returned_null: "strchr" returns null (checked 273 out of 288 times).
Event var_assigned: Assigning: "p" = null return value from
"strchr".
Event cond_true: Condition "(p = strchr(def->bridge, 37)) ==
strrchr(def->bridge, 37)", taking true branch
Event dereference: Dereferencing a null pointer "p".
John
>
> Jan
>>
>> ACK with that simplification.
at which point p is no longer a dead variable, but I've still reduced
the code complexity.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:55 a.m.
On Wed, May 13, 2015 at 06:39:12AM -0400, John Ferlan wrote:
On 05/05/2015 12:39 PM, Eric Blake wrote:
> On 05/05/2015 10:26 AM, Ján Tomko wrote:
>> On Tue, May 05, 2015 at 10:14:18AM -0600, Eric Blake wrote:
>>> On 05/05/2015 10:05 AM, Ján Tomko wrote:
>>>> For some reason, we allow a bridge name with %d in it, which we replace
>>>> with an unsigned integer to form a bridge name that does not yet exist
>>>> on the host.
>>>>
>
>>>> + if (def->bridge &&
>>>> + (p = strchr(def->bridge, '%')) ==
strrchr(def->bridge, '%') &&
>>>> + strstr(def->bridge, "%d"))
>>>
>>> Simpler as:
>>>
>>> if (def->bridge &&
>>> strstr(def->bridge, "%d") == strrchr(def->bridge,
'%'))
>>
>> I still don't see it.
>>
>> [A] strchr(def->bridge, '%')
>> [B] strrchr(def->bridge, '%')
>> [C] strstr(def->bridge, "%d"))
>>
>> When def->bridge is '%s%s%s%d', [A] points to the first %s, [B]
points
>> to the %d and so does [C]
>>
>> This string would pass the simplified condition (B == C), but not the
>> full one (A != C)
>
> Okay, I see your counterargument. Still, strstr() is pretty expensive
> compared to just:
>
> if (def->bridge &&
> (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
> p[1] == 'd')
>
Coverity complains :
Event returned_null: "strchr" returns null (checked 273 out of 288 times).
strchr does not return NULL here because networkFindUnusedBridgeName is
only called if either def->bridge is NULL or def->bridge contains "%d".
Jan
Event var_assigned: Assigning: "p" = null return value
from "strchr".
Event cond_true: Condition "(p = strchr(def->bridge, 37)) ==
strrchr(def->bridge, 37)", taking true branch
Event dereference: Dereferencing a null pointer "p".
John
9:58 a.m.
On 05/13/2015 04:55 AM, Ján Tomko wrote:
>> Okay, I see your counterargument. Still, strstr() is pretty
expensive
>> compared to just:
>>
>> if (def->bridge &&
>> (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
>> p[1] == 'd')
>>
>
> Coverity complains :
>
> Event returned_null: "strchr" returns null (checked 273 out of 288
times).
strchr does not return NULL here because networkFindUnusedBridgeName is
only called if either def->bridge is NULL or def->bridge contains "%d".
Adding "p &&" just before "p[1] == 'd'" should silence
the false
positive, right?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:10 a.m.
On 05/13/2015 10:58 AM, Eric Blake wrote:
On 05/13/2015 04:55 AM, Ján Tomko wrote:
>>> Okay, I see your counterargument. Still, strstr() is pretty expensive
>>> compared to just:
>>>
>>> if (def->bridge &&
>>> (p = strchr(def->bridge, '%')) == strrchr(def->bridge,
'%') &&
>>> p[1] == 'd')
>>>
>>
>> Coverity complains :
>>
>> Event returned_null: "strchr" returns null (checked 273 out of 288
times).
>
> strchr does not return NULL here because networkFindUnusedBridgeName is
> only called if either def->bridge is NULL or def->bridge contains
"%d".
Adding "p &&" just before "p[1] == 'd'" should
silence the false
positive, right?
Yes. I'll send something in a bit - there's another two that are queued
up as well.
John
3484
days inactive
3492
days old
7 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko