On 05/20/2013 01:55 PM, Michal Privoznik wrote:
> ---
> src/rpc/gendispatch.pl | 21 ++++--------
> src/rpc/virnetclient.c | 16 ++++-----
> src/rpc/virnetmessage.c | 27 +++++++++------
> src/rpc/virnetsaslcontext.c | 6 ++--
> src/rpc/virnetserver.c | 6 ++--
> src/rpc/virnetserverclient.c | 10 ++----
> src/rpc/virnetservermdns.c | 6 ++--
> src/rpc/virnetsocket.c | 10 +++---
> src/rpc/virnetsshsession.c | 78 +++++++++++++++++++++-----------------------
> src/rpc/virnettlscontext.c | 26 +++++++--------
> 10 files changed, 92 insertions(+), 114 deletions(-)
>
> diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
[...snip...]
> diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
> index b2c6e5b..8483cd5 100644
> --- a/src/rpc/virnetmessage.c
> +++ b/src/rpc/virnetmessage.c
> @@ -29,6 +29,7 @@
> #include "virlog.h"
> #include "virfile.h"
> #include "virutil.h"
> +#include "virstring.h"
>
> #define VIR_FROM_THIS VIR_FROM_RPC
>
> @@ -514,22 +515,28 @@ void virNetMessageSaveError(virNetMessageErrorPtr rerr)
> if (verr) {
> rerr->code = verr->code;
> rerr->domain = verr->domain;
> - if (verr->message && VIR_ALLOC(rerr->message) == 0)
> - *rerr->message = strdup(verr->message);
> + if (verr->message && VIR_ALLOC(rerr->message) == 0 &&
> + VIR_STRDUP_QUIET(*rerr->message, verr->message) < 0)
> + VIR_FREE(rerr->message);
> rerr->level = verr->level;
> - if (verr->str1 && VIR_ALLOC(rerr->str1) == 0)
> - *rerr->str1 = strdup(verr->str1);
> - if (verr->str2 && VIR_ALLOC(rerr->str2) == 0)
> - *rerr->str2 = strdup(verr->str2);
> - if (verr->str3 && VIR_ALLOC(rerr->str3) == 0)
> - *rerr->str3 = strdup(verr->str3);
> + if (verr->str1 && VIR_ALLOC(rerr->str1) == 0 &&
> + VIR_STRDUP_QUIET(*rerr->str1, verr->str1) < 0)
> + VIR_FREE(verr->str1);
> + if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 &&
> + VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0)
> + VIR_FREE(verr->str2);
> + if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 &&
> + VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0)
> + VIR_FREE(verr->str2);
Coverity has a complaint in here:
525 if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 &&
526 VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0)
(1) Event original: "verr->str2" looks like the original copy.
Also see events: [copy_paste_error]
527 VIR_FREE(verr->str2);
528 if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 &&
529 VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0)
(2) Event copy_paste_error: "str2" in "verr->str2" looks like a
copy-paste error. Should it say "str3" instead?
Also see events: [original]
530 VIR_FREE(verr->str2);
531 rerr->int1 = verr->int1;
532 rerr->int2 = verr->int2;
The complaint is only on the last VIR_FREE(verr->str2); as a cut-n-paste
of the one on line 527; however, it seems to me all those VIR_FREE()'s
should be on "rerr" and not "verr", right?
That is, there's a VIR_ALLOC(rerr->str#) for each section, then on
failure shouldn't it be VIR_FREE(rerr->str#)?
John
> rerr->int1 = verr->int1;
> rerr->int2 = verr->int2;
> } else {
> rerr->code = VIR_ERR_INTERNAL_ERROR;
> rerr->domain = VIR_FROM_RPC;
> - if (VIR_ALLOC(rerr->message) == 0)
> - *rerr->message = strdup(_("Library function returned error but did not set virError"));
> + if (VIR_ALLOC(rerr->message) == 0 &&
> + VIR_STRDUP_QUIET(*rerr->message,
> + _("Library function returned error but did not set virError")) < 0)
> + VIR_FREE(rerr->message);
> rerr->level = VIR_ERR_ERROR;
> }
> }
[...snip...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list