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

Re: [RFC PATCH 1/4] xen/arm: justify or initialize conditionally uninitialized variables





On 20/07/23 17:39, Julien Grall wrote:
Hi,

The e-mail is getting quite long. Can you trim the unnecessary bits when replying?


Ok.

On 20/07/2023 15:23, Nicola Vetrini wrote:
The problem is that _t may be uninitialized, hence assigning its address to t could be problematic.

But the value is set right after. IOW, there is no read between. So how is this prob

Another way to address this is to initialize _t to a bad value and use this variable in the body, then assign to t based on the value just before returning.

IHMO, neither solution are ideal. I think we should investigate whether Eclair can be improved.

[...]


I'll see what can be done about it, I'll reply when I have an answer.


What about this:

-    p2m_type_t _t;
+    p2m_type_t _t = p2m_invalid;
[...]
      t = t ?: &_t;
-    *t = p2m_invalid;
+    *t = _t;

The resulting code is still quite confusing. I am still not quite sure why ECLAIR can't understand the construct. Apologies if this was already said, but this thread is getting quite long with many different issues. So it is a bit difficult to navigate (which is why I suggested to split and have a commit message explaining the rationale for each).

Anyway, if we can't improve Eclair, then my preference would be the following:

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index de32a2d638ba..05d65db01b0c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -496,16 +496,13 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
      lpae_t entry, *table;
      int rc;
      mfn_t mfn = INVALID_MFN;
-    p2m_type_t _t;
      DECLARE_OFFSETS(offsets, addr);

      ASSERT(p2m_is_locked(p2m));
      BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);

-    /* Allow t to be NULL */
-    t = t ?: &_t;
-
-    *t = p2m_invalid;
+    if ( t )
+        *t = p2m_invalid;

      if ( valid )
          *valid = false;
@@ -549,7 +546,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,

      if ( p2m_is_valid(entry) )
      {
-        *t = entry.p2m.type;
+        if ( t )
+            *t = entry.p2m.type;

          if ( a )
              *a = p2m_mem_access_radix_get(p2m, gfn);


Ok, I'll make a separate patch.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.