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