[libvirt] [PATCH] 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. --- This was added by commit 4c3f3b4 http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=4c3f3b4 https://www.redhat.com/archives/libvir-list/2009-April/msg00394.html claiming it worked some time before that src/network/bridge_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3b879cd..1c5613a 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) @@ -2809,7 +2815,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, /* * networkValidateBridgeName() - if no bridge name is set, or if the - * bridge name contains a %d (indicating that this is a template for + * bridge name contains a % (indicating that this is a template for * the actual name) try to set an appropriate bridge name. If a * bridge name *is* set, make sure it doesn't conflict with any other * network's bridge name. @@ -2820,7 +2826,7 @@ networkBridgeNameValidate(virNetworkObjListPtr nets, { int ret = -1; - if (def->bridge && !strstr(def->bridge, "%d")) { + if (def->bridge && !strchr(def->bridge, '%')) { if (virNetworkBridgeInUse(nets, def->bridge, def->name)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge name '%s' already in use."), -- 2.0.5

On 04/30/2015 06:28 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. ---
+ if (def->bridge && + (p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%') && + strstr(def->bridge, "%d"))
Shorter: if (def->bridge && strstr(def->bridge, "%d") == strrchr(def->bridge, '%'))
@@ -2809,7 +2815,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
/* * networkValidateBridgeName() - if no bridge name is set, or if the - * bridge name contains a %d (indicating that this is a template for + * bridge name contains a % (indicating that this is a template for
No need to change this comment any more.
* the actual name) try to set an appropriate bridge name. If a * bridge name *is* set, make sure it doesn't conflict with any other * network's bridge name. @@ -2820,7 +2826,7 @@ networkBridgeNameValidate(virNetworkObjListPtr nets, { int ret = -1;
- if (def->bridge && !strstr(def->bridge, "%d")) { + if (def->bridge && !strchr(def->bridge, '%')) {
I'm also not sure we need to change this. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Thu, Apr 30, 2015 at 08:21:34AM -0600, Eric Blake wrote:
On 04/30/2015 06:28 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. ---
+ if (def->bridge && + (p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%') && + strstr(def->bridge, "%d"))
Shorter:
if (def->bridge && strstr(def->bridge, "%d") == strrchr(def->bridge, '%'))
This only checks if there are no '%' characters after the first "%d", it would still let "br%s%d" through.
@@ -2809,7 +2815,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
/* * networkValidateBridgeName() - if no bridge name is set, or if the - * bridge name contains a %d (indicating that this is a template for + * bridge name contains a % (indicating that this is a template for
No need to change this comment any more.
* the actual name) try to set an appropriate bridge name. If a * bridge name *is* set, make sure it doesn't conflict with any other * network's bridge name. @@ -2820,7 +2826,7 @@ networkBridgeNameValidate(virNetworkObjListPtr nets, { int ret = -1;
- if (def->bridge && !strstr(def->bridge, "%d")) { + if (def->bridge && !strchr(def->bridge, '%')) {
I'm also not sure we need to change this.
Right, this is only used with STREQ. Jan
participants (2)
-
Eric Blake
-
Ján Tomko