| Message ID | 1448091903-14460-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
21.11.2015 10:45, Cao jin wrote: > add param check for pci_add_capability2, as it is a public API. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/pci/pci.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 168b9cc..6938f64 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id, > uint8_t *config; > int i, overlapping_cap; > > + assert(size > 0); > + assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset); > + I'd like to see some ACKs/Reviews for this one, in particular why size should be != 0. Also either move offset assert to the below "else" clause or rewrite it to be offset == 0 instead if !offset :) Thanks, /mjt > if (!offset) { > offset = pci_find_space(pdev, size); > if (!offset) { >
On 11/01/2016 09:32, Michael Tokarev wrote: >> > >> > + assert(size > 0); >> > + assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset); >> > + > I'd like to see some ACKs/Reviews for this one, in particular why > size should be != 0. In fact it should be >= 2, because two bytes are always written below: config = pdev->config + offset; config[PCI_CAP_LIST_ID] = cap_id; config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST]; > Also either move offset assert to the below > "else" clause or rewrite it to be offset == 0 instead if !offset :) Good idea to move it below, or even to add assert(offset >= PCI_CONFIG_HEADER_SIZE); after the "if", before the "config" assignment. Paolo
On 01/11/2016 04:32 PM, Michael Tokarev wrote: > 21.11.2015 10:45, Cao jin wrote: >> add param check for pci_add_capability2, as it is a public API. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/pci/pci.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 168b9cc..6938f64 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id, >> uint8_t *config; >> int i, overlapping_cap; >> >> + assert(size > 0); >> + assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset); >> + > > I'd like to see some ACKs/Reviews for this one, in particular why > size should be != 0. see pci_find_space(), if size == 0, I guess it will always return 0. But I should admit that I did made a mistake, I will talk about it in next mail. Also either move offset assert to the below > "else" clause or rewrite it to be offset == 0 instead if !offset :) > > Thanks, > > /mjt > >> if (!offset) { >> offset = pci_find_space(pdev, size); >> if (!offset) { >> > > > > . >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 168b9cc..6938f64 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2144,6 +2144,9 @@ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id, uint8_t *config; int i, overlapping_cap; + assert(size > 0); + assert(offset >= PCI_CONFIG_HEADER_SIZE || !offset); + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) {
add param check for pci_add_capability2, as it is a public API. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/pci/pci.c | 3 +++ 1 file changed, 3 insertions(+)