[PATCH v3] hw/scsi/ncr53c710: Fixing defects reported by Coverity Scan for QEMU

[PATCH v3] hw/scsi/ncr53c710: Fixing defects reported by Coverity Scan for QEMU

[Soumyajyotii Ssarkar]

From: RemZapCypher <soumyajyotisarkar23@gmail.com>

Fixing Null pointer dereference & Async/Sync IDENTICAL_BRANCHES

Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
---
 hw/scsi/ncr53c710.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c
index b3d4593b72..dd9884c8bf 100644
--- a/hw/scsi/ncr53c710.c
+++ b/hw/scsi/ncr53c710.c
@@ -834,13 +834,11 @@ void ncr710_transfer_data(SCSIRequest *req, uint32_t len)
         }
     }
 
-    /* Host adapter (re)connected */
-    s->current->dma_len = len;
     s->command_complete = NCR710_CMD_DATA_READY;
-
     if (!s->current) {
-        return;
+        s->current = (NCR710Request *)req->hba_private;
     }
+    s->current->dma_len = len;
 
     if (s->waiting) {
         s->scntl1 |= NCR710_SCNTL1_CON;
@@ -1367,11 +1365,6 @@ again:
         case PHASE_DI:
             s->waiting = NCR710_WAIT_DMA;
             ncr710_do_dma(s, 0);
-            if (s->waiting != NCR710_WAIT_NONE) {
-                /* Async - stop and wait */
-                break;
-            }
-            /* Sync - continue execution */
             break;
         case PHASE_CO:
             ncr710_do_command(s);
-- 
2.49.0

Re: [PATCH v3] hw/scsi/ncr53c710: Fixing defects reported by Coverity Scan for QEMU

[Peter Maydell]

On Sat, 1 Nov 2025 at 08:23, Soumyajyotii Ssarkar
<soumyajyotisarkar23@gmail.com> wrote:
>
> From: RemZapCypher <soumyajyotisarkar23@gmail.com>
>
> Fixing Null pointer dereference & Async/Sync IDENTICAL_BRANCHES

Commit messages should generally try to explain what the
patch is doing in more detail than this. Ideally I should
be able to understand from the commit message why the
change makes sense and is the right way to fix the bug.

> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> ---
>  hw/scsi/ncr53c710.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/hw/scsi/ncr53c710.c b/hw/scsi/ncr53c710.c
> index b3d4593b72..dd9884c8bf 100644
> --- a/hw/scsi/ncr53c710.c
> +++ b/hw/scsi/ncr53c710.c
> @@ -834,13 +834,11 @@ void ncr710_transfer_data(SCSIRequest *req, uint32_t len)
>          }
>      }
>
> -    /* Host adapter (re)connected */
> -    s->current->dma_len = len;
>      s->command_complete = NCR710_CMD_DATA_READY;
> -
>      if (!s->current) {
> -        return;
> +        s->current = (NCR710Request *)req->hba_private;
>      }
> +    s->current->dma_len = len;

This looks odd. We already have code (the s->wait_reselect
check) earlier that says "in this situation, we need to
update s->current with req_hba_private and update s->current_len".
Now here we're making this code unconditionally do "if s->current
isn't set, set it from hba_private". This seems like it's
confused about when s->current is valid and when it's not.

When we call this function:
 * when is s->current non-NULL and valid to use?
 * when should we be updating s->current from req->hba_private?
 * when should we update s->current->dma_len?

I think that either:
 (a) there's a particular set of conditions like s->wait_reselect
     that tell us when we should update s->current, and we
     only need to do that then, or
 (b) we need to do it basically unconditionally

We shouldn't have both "do this on a particular
condition" and then "do this always".

>      if (s->waiting) {
>          s->scntl1 |= NCR710_SCNTL1_CON;
> @@ -1367,11 +1365,6 @@ again:
>          case PHASE_DI:
>              s->waiting = NCR710_WAIT_DMA;
>              ncr710_do_dma(s, 0);
> -            if (s->waiting != NCR710_WAIT_NONE) {
> -                /* Async - stop and wait */
> -                break;
> -            }
> -            /* Sync - continue execution */
>              break;
>          case PHASE_CO:
>              ncr710_do_command(s);
> --
> 2.49.0

Please don't fix two unrelated issues in the same commit:
these should be separate patches (you can send them as
part of a 2-patch patchset).

thanks
-- PMM