From 1a214458770ff7d79188d25bb7b27fd0786d933e Mon Sep 17 00:00:00 2001 From: ligd Date: Wed, 22 Nov 2023 17:59:30 +0800 Subject: [PATCH] pipes: use priv refs instead of inode to resolve memleak thread1 thread2 open pipe open pipe close() -> pipecommon_close() -> check inode refs -> do NOT free dev close() -> pipecommon_close() -> check inode refs -> do NOT free dev -> inode_release inode refs-- -> inode_release inode refs-- Then, you will see the pipe hasn't free its resource, memleak Resolve: replace the inode refs with priv refs Signed-off-by: ligd --- drivers/pipes/pipe.c | 31 +++------------------------- drivers/pipes/pipe_common.c | 40 ++++++++++++++++++++----------------- drivers/pipes/pipe_common.h | 1 + 3 files changed, 26 insertions(+), 46 deletions(-) diff --git a/drivers/pipes/pipe.c b/drivers/pipes/pipe.c index 4220db4d7a..caf0394d1f 100644 --- a/drivers/pipes/pipe.c +++ b/drivers/pipes/pipe.c @@ -48,8 +48,6 @@ * Private Function Prototypes ****************************************************************************/ -static int pipe_close(FAR struct file *filep); - static int pipe_mmap(FAR struct file *filep, FAR struct mm_map_entry_s *entry); @@ -60,7 +58,7 @@ static int pipe_mmap(FAR struct file *filep, static const struct file_operations g_pipe_fops = { pipecommon_open, /* open */ - pipe_close, /* close */ + pipecommon_close, /* close */ pipecommon_read, /* read */ pipecommon_write, /* write */ NULL, /* seek */ @@ -101,31 +99,6 @@ static inline int pipe_allocate(void) return ret; } -/**************************************************************************** - * Name: pipe_close - ****************************************************************************/ - -static int pipe_close(FAR struct file *filep) -{ - FAR struct inode *inode = filep->f_inode; - FAR struct pipe_dev_s *dev = inode->i_private; - int ret; - - DEBUGASSERT(dev); - - /* Perform common close operations */ - - ret = pipecommon_close(filep); - if (ret == 0 && inode->i_crefs == 1) - { - /* Release the pipe when there are no further open references to it. */ - - pipecommon_freedev(dev); - } - - return ret; -} - /**************************************************************************** * Name: pipe_mmap ****************************************************************************/ @@ -170,6 +143,8 @@ static int pipe_register(size_t bufsize, int flags, return -ENOMEM; } + PIPE_UNLINK(dev->d_flags); + /* Register the pipe device */ ret = register_pipedriver(devname, &g_pipe_fops, 0666, (FAR void *)dev); diff --git a/drivers/pipes/pipe_common.c b/drivers/pipes/pipe_common.c index 8b0452b4c6..a47db19d63 100644 --- a/drivers/pipes/pipe_common.c +++ b/drivers/pipes/pipe_common.c @@ -94,12 +94,11 @@ FAR struct pipe_dev_s *pipecommon_allocdev(size_t bufsize) /* Allocate a private structure to manage the pipe */ - dev = kmm_malloc(sizeof(struct pipe_dev_s)); + dev = kmm_zalloc(sizeof(struct pipe_dev_s)); if (dev) { /* Initialize the private structure */ - memset(dev, 0, sizeof(struct pipe_dev_s)); nxmutex_init(&dev->d_bflock); nxsem_init(&dev->d_rdsem, 0, 0); nxsem_init(&dev->d_wrsem, 0, 0); @@ -150,7 +149,7 @@ int pipecommon_open(FAR struct file *filep) * is first opened. */ - if (inode->i_crefs == 1 && !circbuf_is_init(&dev->d_buffer)) + if (dev->d_crefs == 0) { ret = circbuf_init(&dev->d_buffer, NULL, dev->d_bufsize); if (ret < 0) @@ -160,6 +159,8 @@ int pipecommon_open(FAR struct file *filep) } } + dev->d_crefs++; + /* If opened for writing, increment the count of writers on the pipe * instance. */ @@ -304,7 +305,7 @@ int pipecommon_close(FAR struct file *filep) FAR struct pipe_dev_s *dev = inode->i_private; int ret; - DEBUGASSERT(dev && filep->f_inode->i_crefs > 0); + DEBUGASSERT(dev && dev->d_crefs > 0); /* Make sure that we have exclusive access to the device structure. * NOTE: close() is supposed to return EINTR if interrupted, however @@ -325,7 +326,8 @@ int pipecommon_close(FAR struct file *filep) /* Check if the decremented inode reference count would go to zero */ - if (inode->i_crefs > 1) + dev->d_crefs--; + if (dev->d_crefs > 0) { /* More references.. If opened for writing, decrement the count of * writers on the pipe instance. @@ -886,26 +888,28 @@ int pipecommon_ioctl(FAR struct file *filep, int cmd, unsigned long arg) int pipecommon_unlink(FAR struct inode *inode) { FAR struct pipe_dev_s *dev; + int ret; DEBUGASSERT(inode->i_private); dev = inode->i_private; + ret = nxmutex_lock(&dev->d_bflock); + if (ret < 0) + { + return ret; + } + + if (dev->d_crefs <= 0) + { + circbuf_uninit(&dev->d_buffer); + pipecommon_freedev(dev); + return OK; + } + /* Mark the pipe unlinked */ PIPE_UNLINK(dev->d_flags); - - /* Are the any open references to the driver? */ - - if (inode->i_crefs == 1) - { - /* No.. free the buffer (if there is one) */ - - circbuf_uninit(&dev->d_buffer); - - /* And free the device structure. */ - - pipecommon_freedev(dev); - } + nxmutex_unlock(&dev->d_bflock); return OK; } diff --git a/drivers/pipes/pipe_common.h b/drivers/pipes/pipe_common.h index 72f1af89f5..77f0b42fcb 100644 --- a/drivers/pipes/pipe_common.h +++ b/drivers/pipes/pipe_common.h @@ -126,6 +126,7 @@ struct pipe_dev_s uint8_t d_nwriters; /* Number of reference counts for write access */ uint8_t d_nreaders; /* Number of reference counts for read access */ uint8_t d_flags; /* See PIPE_FLAG_* definitions */ + int16_t d_crefs; /* References to dev */ struct circbuf_s d_buffer; /* Buffer allocated when device opened */ /* The following is a list if poll structures of threads waiting for