[dm-devel] 答复: Re: [PATCH] libmultipath: fix memory API logic error

[tang.junhui at zte.com.cn]

Hi, Johannes,
Your advice is nice, thank you
I will send the modified patch later.




发件人:         Johannes Thumshirn <jthumshirn at suse.de>
收件人:         tang.junhui at zte.com.cn, 
抄送:   christophe varoqui <christophe.varoqui at free.fr>, 
zhang.kai16 at zte.com.cn, dm-devel at redhat.com
日期:   2016/08/15 21:59
主题:   Re: [dm-devel] [PATCH] libmultipath: fix memory API logic error



On Mon, Aug 15, 2016 at 06:59:09PM +0800, tang.junhui at zte.com.cn wrote:
> From: "tang.junhui" <tang.junhui at zte.com.cn>
> 
> Memroy API use mem_allocated to record the total size of used memory,
> however, it's wrong to use size(p) as the length of freed memory in 
xfree(),
> and memory may also be allocated by STRDUP() or REALLOC(), which is
> not calculated into mem_allocated, so the total size of used memory is
> not correctly. For these reasons, we removed these incorrectly code to 
keep
> the code clean.
> 
> Signed-off-by: tang.junhui <tang.junhui at zte.com.cn>
> ---
>  libmpathpersist/mpath_updatepr.c |  1 -
>  libmultipath/memory.c            | 31 -------------------------------
>  libmultipath/memory.h            | 13 ++-----------
>  3 files changed, 2 insertions(+), 43 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_updatepr.c 
b/libmpathpersist/mpath_updatepr.c
> index 9ff4b30..5af2e03 100644
> --- a/libmpathpersist/mpath_updatepr.c
> +++ b/libmpathpersist/mpath_updatepr.c
> @@ -16,7 +16,6 @@
>  #include "uxsock.h"
>  #include "memory.h"
> 
> -unsigned long mem_allocated;    /* Total memory used in Bytes */
> 
>  int update_prflag(char * arg1, char * arg2, int noisy)
>  {
> diff --git a/libmultipath/memory.c b/libmultipath/memory.c
> index 1366f45..5441e6a 100644
> --- a/libmultipath/memory.c
> +++ b/libmultipath/memory.c
> @@ -22,37 +22,6 @@
>

[...]

> -
> -void
> -xfree(void *p)
> -{
> -              mem_allocated -= sizeof (p);
> -              free(p);
> -              p = NULL;
> -}
> -
>  /*

[...]

> -#define MALLOC(n)    (zalloc(n))
> -#define FREE(p)      (xfree(p))
> +#define MALLOC(n)    (calloc(1,(n)))
> +#define FREE(p)      (free(p))
>  #define REALLOC(p,n) (realloc((p),(n)))
>  #define STRDUP(n)    (strdup(n))

One minor nit, the original xfree() has set the freed pointer to 0, so 
maybe a

#define FREE(p) do { free(p); p = NULL } while(0)

would be advisable. I personally like setting free'd pointers to NULL,
as a NULL pointer access is a more immediate feedback than some rather
nasty use-after-free.

Just my $0.2

Byte,
                 Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20160816/e13d2055/attachment.htm>