[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable



On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long variable
but is stored in a unsigned short. If the actual number was too large,
a faulty number was stored.
---
  src/conf/domain_conf.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0b9ba13..3e95ec9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
          def->maxvcpus = 1;
      } else {
          def->maxvcpus = count;
-        if (count == 0) {
+        if (count == 0 || (unsigned short) count != count) {

maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if
def->maxvcpus != count after this point, then we have the overflow,
right?  Or would the compiler "adjust" that comparison behind our back
on an if check?

You mean changing the explicit typecast with checking of def->maxvcpus?

This works in gcc too, but I'm afraid I have -O0 as a default for libvirt. I'm not sure if a compiler is allowed to optimize such a comparison, but the explicit typecast is probably safer and it is more noticeable to a possible future reader of that piece of code.


              virReportError(VIR_ERR_XML_ERROR,
                             _("invalid maxvcpus %lu"), count);
              goto error;
@@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
          def->vcpus = def->maxvcpus;
      } else {
          def->vcpus = count;
-        if (count == 0) {
+        if (count == 0 || (unsigned short) count != count) {

Same comment as 'maxvcpus'

              virReportError(VIR_ERR_XML_ERROR,
                             _("invalid current vcpus %lu"), count);
              goto error;


ACK - I think what you've done is right, although perhaps someone with a
bit more knowledge of what the compiler does could pipe in (I'm curious
too).

For now, I will probably push this as is. We still can fix this in the future if there will be a cleaner solution.

Thanks for the review.

Peter

John



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]