migration/multifd: Fix loop conditions in multifd_zstd_send_prepare and multifd_zstd_recv

[PATCH] migration/multifd: Fix loop conditions in multifd_zstd_send_prepare and multifd_zstd_recv

Posted by Stefan Weil via 1 year, 5 months ago

GitHub's CodeQL reports four critical errors which are fixed by this commit:

    Unsigned difference expression compared to zero

An expression (u - v > 0) with unsigned values u, v is only false if u == v,
so all changed expressions did not work as expected.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

I don't know what effect the errors will have.
Please check whether the fix should be backported to existing versions of QEMU.

And I think that it might be a good idea to add the security check to
https://github.com/qemu/qemu, too. The critical errors here and in net/colo-compare.c
were not reported by other static code analyzers as far as I know.
Paolo, if desired, I can send a patch for CodeQL.

Stefan W.

 migration/multifd-zstd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
index 53da33e048..abed140855 100644
--- a/migration/multifd-zstd.c
+++ b/migration/multifd-zstd.c
@@ -123,9 +123,9 @@ static int multifd_zstd_send_prepare(MultiFDSendParams *p, Error **errp)
          */
         do {
             ret = ZSTD_compressStream2(z->zcs, &z->out, &z->in, flush);
-        } while (ret > 0 && (z->in.size - z->in.pos > 0)
-                         && (z->out.size - z->out.pos > 0));
-        if (ret > 0 && (z->in.size - z->in.pos > 0)) {
+        } while (ret > 0 && (z->in.size > z->in.pos)
+                         && (z->out.size > z->out.pos));
+        if (ret > 0 && (z->in.size > z->in.pos)) {
             error_setg(errp, "multifd %u: compressStream buffer too small",
                        p->id);
             return -1;
@@ -243,7 +243,7 @@ static int multifd_zstd_recv(MultiFDRecvParams *p, Error **errp)
          */
         do {
             ret = ZSTD_decompressStream(z->zds, &z->out, &z->in);
-        } while (ret > 0 && (z->in.size - z->in.pos > 0)
+        } while (ret > 0 && (z->in.size > z->in.pos)
                          && (z->out.pos < page_size));
         if (ret > 0 && (z->out.pos < page_size)) {
             error_setg(errp, "multifd %u: decompressStream buffer too small",
-- 
2.39.2

Re: [PATCH] migration/multifd: Fix loop conditions in multifd_zstd_send_prepare and multifd_zstd_recv

Posted by Peter Xu 1 year, 5 months ago

On Tue, Sep 10, 2024 at 07:41:38AM +0200, Stefan Weil wrote:
> GitHub's CodeQL reports four critical errors which are fixed by this commit:
> 
>     Unsigned difference expression compared to zero
> 
> An expression (u - v > 0) with unsigned values u, v is only false if u == v,
> so all changed expressions did not work as expected.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> I don't know what effect the errors will have.
> Please check whether the fix should be backported to existing versions of QEMU.
> 
> And I think that it might be a good idea to add the security check to
> https://github.com/qemu/qemu, too. The critical errors here and in net/colo-compare.c
> were not reported by other static code analyzers as far as I know.
> Paolo, if desired, I can send a patch for CodeQL.

I hope Paolo can see these lines.

Patch queued, thanks.

-- 
Peter Xu