Re: [libvirt] [PATCH] cpu: fix possible crash in getModels

On 12/03/14 19:02, Pavel Hrdina wrote: > Commit 86a15a25 introduced a new cpu driver API 'getModels'. Public API > allow you to pass NULL for models to get only number of existing models. > However the new code will crash with segfault so we have to count with > the possibility that the user wants only the number. > > There is also difference in order of the models gathered by this new API > as the old approach was inserting the elements to the end of the array > so we should use 'VIR_APPEND_ELEMENT'. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/cpu/cpu_powerpc.c | 15 ++++++++++----- > src/cpu/cpu_x86.c | 15 ++++++++++----- > 2 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c > index 871401b..86f80b3 100644 > --- a/src/cpu/cpu_powerpc.c > +++ b/src/cpu/cpu_powerpc.c > @@ -666,11 +666,15 @@ ppcGetModels(char ***models) > > model = map->models; > while (model != NULL) { > - if (VIR_STRDUP(name, model->name) < 0) > - goto error; > + if (models) { > + if (VIR_STRDUP(name, model->name) < 0) > + goto error; > > - if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0) > - goto error; > + if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) > + goto error; > + } else { > + nmodels++; > + } > > model = model->next; > } > @@ -681,7 +685,8 @@ ppcGetModels(char ***models) > return nmodels; > > error: > - virStringFreeList(*models); > + if (models) > + virStringFreeList(*models); if (models) { virStringFreeList(*models); *models = NULL; } To remove the possibly invalid pointer. > nmodels = -1; > goto cleanup; > } > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c > index f6dcba4..dfbc16c 100644 > --- a/src/cpu/cpu_x86.c > +++ b/src/cpu/cpu_x86.c > @@ -2176,11 +2176,15 @@ x86GetModels(char ***models) > > model = map->models; > while (model != NULL) { > - if (VIR_STRDUP(name, model->name) < 0) > - goto error; > + if (models) { > + if (VIR_STRDUP(name, model->name) < 0) > + goto error; > > - if (VIR_INSERT_ELEMENT(*models, 0, nmodels, name) < 0) > - goto error; > + if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) > + goto error; > + } else { > + nmodels++; > + } > > model = model->next; > } > @@ -2188,7 +2192,8 @@ x86GetModels(char ***models) > return nmodels; > > error: > - virStringFreeList(*models); > + if (models) > + virStringFreeList(*models); Same as above. > return -1; > } > > ACK, Peter