[libvirt] [PATCHv2] Ignore bridge template names with multiple printf conversions
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
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
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
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
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko