xiaoxiang781216 commented on code in PR #19704:
URL: https://github.com/apache/nuttx/pull/19704#discussion_r3725922345
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -169,25 +170,67 @@ static int pseudofile_close(FAR struct file *filep)
return OK;
}
+/* Round allocation up to a power of two when safe. For sizes that cannot
+ * be represented as a shifted size_t (or where rounding would overflow),
+ * allocate the exact requested size instead.
+ */
+
+static size_t pseudofile_buffersize(size_t size)
+{
+ size_t alloc;
+
+ if (size == 0)
+ {
+ return 1;
+ }
+
+ if (size > (SIZE_MAX >> 1))
+ {
+ return size;
+ }
+
+ alloc = ((size_t)1) << LOG2_CEIL(size);
Review Comment:
why not simply double size? LOG2_CEIL isn't a fast operation.
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -207,7 +251,14 @@ static ssize_t pseudofile_write(FAR struct file *filep,
if (filep->f_oflags & O_APPEND)
{
- ret = pseudofile_expand(node, node->i_size + buflen);
+ if (buflen > SIZE_MAX - node->i_size)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EFBIG;
+ }
+
+ endpos = node->i_size + buflen;
+ ret = pseudofile_expand(node, endpos);
Review Comment:
```suggestion
ret = pseudofile_expand(node, node->i_size + buflen);
```
and remove endpos
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -169,25 +170,67 @@ static int pseudofile_close(FAR struct file *filep)
return OK;
}
+/* Round allocation up to a power of two when safe. For sizes that cannot
+ * be represented as a shifted size_t (or where rounding would overflow),
+ * allocate the exact requested size instead.
+ */
+
+static size_t pseudofile_buffersize(size_t size)
+{
+ size_t alloc;
+
+ if (size == 0)
+ {
+ return 1;
+ }
+
+ if (size > (SIZE_MAX >> 1))
+ {
+ return size;
+ }
+
+ alloc = ((size_t)1) << LOG2_CEIL(size);
+ if (alloc < size)
+ {
+ return size;
+ }
+
+ return alloc;
+}
+
static int pseudofile_expand(FAR struct inode *node,
size_t size)
{
FAR struct fs_pseudofile_s *pf = node->i_private;
FAR void *tmp;
+ size_t oldsize = node->i_size;
+ size_t alloc;
- if (pf->content && fs_heap_malloc_size(pf->content) >= size)
+ if (pf->content != NULL && fs_heap_malloc_size(pf->content) >= size)
{
+ if (size > oldsize)
+ {
+ memset(pf->content + oldsize, 0, size - oldsize);
+ }
+
node->i_size = size;
return 0;
}
- tmp = fs_heap_realloc(pf->content, 1 << LOG2_CEIL(size));
+ alloc = pseudofile_buffersize(size);
+ tmp = fs_heap_realloc(pf->content, alloc);
Review Comment:
```suggestion
tmp = fs_heap_realloc(pf->content, pseudofile_buffersize(size));
```
and remove alloc
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -218,14 +269,34 @@ static ssize_t pseudofile_write(FAR struct file *filep,
}
else
{
- ret = pseudofile_expand(node, filep->f_pos + buflen);
+ if (filep->f_pos < 0)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EINVAL;
+ }
+
+ if ((size_t)filep->f_pos > SIZE_MAX - buflen)
Review Comment:
remove (size_t)
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -218,14 +269,34 @@ static ssize_t pseudofile_write(FAR struct file *filep,
}
else
{
- ret = pseudofile_expand(node, filep->f_pos + buflen);
+ if (filep->f_pos < 0)
Review Comment:
why need check
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -218,14 +269,34 @@ static ssize_t pseudofile_write(FAR struct file *filep,
}
else
{
- ret = pseudofile_expand(node, filep->f_pos + buflen);
+ if (filep->f_pos < 0)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EINVAL;
+ }
+
+ if ((size_t)filep->f_pos > SIZE_MAX - buflen)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EFBIG;
+ }
+
+ endpos = (size_t)filep->f_pos + buflen;
Review Comment:
remove the cast
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -392,29 +463,35 @@ static int pseudofile_truncate(FAR struct file *filep,
off_t length)
return ret;
}
- if (length < node->i_size)
+ if (length < 0)
+ {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if ((size_t)length < node->i_size)
{
FAR void *tmp;
- tmp = fs_heap_realloc(pf->content, length);
+ tmp = fs_heap_realloc(pf->content, (size_t)length);
Review Comment:
```suggestion
tmp = fs_heap_realloc(pf->content, length);
```
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -392,29 +463,35 @@ static int pseudofile_truncate(FAR struct file *filep,
off_t length)
return ret;
}
- if (length < node->i_size)
+ if (length < 0)
+ {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if ((size_t)length < node->i_size)
{
FAR void *tmp;
- tmp = fs_heap_realloc(pf->content, length);
+ tmp = fs_heap_realloc(pf->content, (size_t)length);
if (tmp == NULL)
{
ret = -ENOMEM;
goto out;
}
pf->content = tmp;
- node->i_size = length;
+ node->i_size = (size_t)length;
}
else
{
- ret = pseudofile_expand(node, length);
+ /* Expansion also clears any newly addressed bytes. */
+
+ ret = pseudofile_expand(node, (size_t)length);
Review Comment:
```suggestion
ret = pseudofile_expand(node, length);
```
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -218,14 +269,34 @@ static ssize_t pseudofile_write(FAR struct file *filep,
}
else
{
- ret = pseudofile_expand(node, filep->f_pos + buflen);
+ if (filep->f_pos < 0)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EINVAL;
+ }
+
+ if ((size_t)filep->f_pos > SIZE_MAX - buflen)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EFBIG;
+ }
+
+ endpos = (size_t)filep->f_pos + buflen;
+ ret = pseudofile_expand(node, endpos);
Review Comment:
```suggestion
ret = pseudofile_expand(node, filep->f_pos + buflen);
```
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -392,29 +463,35 @@ static int pseudofile_truncate(FAR struct file *filep,
off_t length)
return ret;
}
- if (length < node->i_size)
+ if (length < 0)
+ {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if ((size_t)length < node->i_size)
Review Comment:
remove the cast
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -218,14 +269,34 @@ static ssize_t pseudofile_write(FAR struct file *filep,
}
else
{
- ret = pseudofile_expand(node, filep->f_pos + buflen);
+ if (filep->f_pos < 0)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EINVAL;
+ }
+
+ if ((size_t)filep->f_pos > SIZE_MAX - buflen)
+ {
+ nxmutex_unlock(&pf->lock);
+ return -EFBIG;
+ }
+
+ endpos = (size_t)filep->f_pos + buflen;
+ ret = pseudofile_expand(node, endpos);
if (ret < 0)
{
nxmutex_unlock(&pf->lock);
return ret;
}
}
+ if (pf->content == NULL ||
+ fs_heap_malloc_size(pf->content) < ((size_t)filep->f_pos + buflen))
Review Comment:
why add this check
##########
fs/vfs/fs_pseudofile.c:
##########
@@ -392,29 +463,35 @@ static int pseudofile_truncate(FAR struct file *filep,
off_t length)
return ret;
}
- if (length < node->i_size)
+ if (length < 0)
+ {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if ((size_t)length < node->i_size)
{
FAR void *tmp;
- tmp = fs_heap_realloc(pf->content, length);
+ tmp = fs_heap_realloc(pf->content, (size_t)length);
if (tmp == NULL)
{
ret = -ENOMEM;
goto out;
}
pf->content = tmp;
- node->i_size = length;
+ node->i_size = (size_t)length;
Review Comment:
revert the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
