On Fri, 12 Aug 2022 at 15:41, Denis Krienbühl <[email protected]> wrote: > > I’m not much further with my segfault, though I now know that the number of > detaches likely does not matter and it seems to occur during the attach, not > the detach part of the code. > > I adapted my change to be a bit more sane - I think it might make sense in > general, as something is clearly wrong, the code can be reached somehow and > in this case we probably just want to stop, instead of pretending everything > is okay. > > So the following change also works for us, causing no segfaults: > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index efee6739f9..7273cd6c3d 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -775,6 +775,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, > uint8_t *outbuf) > return -1; > } > > + /* Avoid null-pointers leading to segfaults below */ > + if (!s->version) { > + return -1; > + } > + > + if (!s->vendor) { > + return -1; > + } > + > /* PAGE CODE == 0 */ > buflen = req->cmd.xfer; > if (buflen > SCSI_MAX_INQUIRY_LEN) { > > I still hope to get some feedback from anyone that is familiar with hw/scsi. > Hopefully this reaches someone who can shed some light on this.
As I said previously, this is still absolutely wrong. If we ever get to this function with either of these fields being NULL then there has been a serious problem, probably a memory corruption or use-after-free, or possibly an attempt to use a partially constructed object. You need to find out why these fields have ended up NULL, not just paper over the problem. You could put in assert(s->version); assert(s->vendor); if you like, though that doesn't really gain much over just segfaulting. thanks -- PMM
