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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




发件人:         Johannes Thumshirn <jthumshirn@xxxxxxx>
收件人:         tang.junhui@xxxxxxxxxx,
抄送:        christophe varoqui <christophe.varoqui@xxxxxxx>, zhang.kai16@xxxxxxxxxx, dm-devel@xxxxxxxxxx
日期:         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@xxxxxxxxxx wrote:
> From: "tang.junhui" <tang.junhui@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxx                                +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

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel

[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux