| Message ID | YJovRPdOiaU6I+JK@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: fix snprintf() checking | expand |
On 5/11/21 3:16 PM, Dan Carpenter wrote: > The snprintf() function returns the number of bytes which would have > been printed if the buffer was large enough. In other words it can > return ">= remain" but this code assumes it returns "== remain". > > The run time impact of this bug is not very severe. The next iteration > through the loop would trigger a WARN() when we pass a negative limit > to snprintf(). We would then return success instead of -E2BIG. > > The kernel implementation of snprintf() will never return negatives so > there is no need to check and I have deleted that dead code. > > Fixes: a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Looks good. But the last 2 sections are introduced by: 74ae4e104dfc ocfs2: Create stack glue sysfs files. With 'Fixes' tag updated, Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > --- > fs/ocfs2/filecheck.c | 6 +----- > fs/ocfs2/stackglue.c | 8 ++------ > 2 files changed, 3 insertions(+), 11 deletions(-) > > diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c > index 90b8d300c1ee..de56e6231af8 100644 > --- a/fs/ocfs2/filecheck.c > +++ b/fs/ocfs2/filecheck.c > @@ -326,11 +326,7 @@ static ssize_t ocfs2_filecheck_attr_show(struct kobject *kobj, > ret = snprintf(buf + total, remain, "%lu\t\t%u\t%s\n", > p->fe_ino, p->fe_done, > ocfs2_filecheck_error(p->fe_status)); > - if (ret < 0) { > - total = ret; > - break; > - } > - if (ret == remain) { > + if (ret >= remain) { > /* snprintf() didn't fit */ > total = -E2BIG; > break; > diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c > index d50e8b8dfea4..16f1bfc407f2 100644 > --- a/fs/ocfs2/stackglue.c > +++ b/fs/ocfs2/stackglue.c > @@ -500,11 +500,7 @@ static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj, > list_for_each_entry(p, &ocfs2_stack_list, sp_list) { > ret = snprintf(buf, remain, "%s\n", > p->sp_name); > - if (ret < 0) { > - total = ret; > - break; > - } > - if (ret == remain) { > + if (ret >= remain) { > /* snprintf() didn't fit */ > total = -E2BIG; > break; > @@ -531,7 +527,7 @@ static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj, > if (active_stack) { > ret = snprintf(buf, PAGE_SIZE, "%s\n", > active_stack->sp_name); > - if (ret == PAGE_SIZE) > + if (ret >= PAGE_SIZE) > ret = -E2BIG; > } > spin_unlock(&ocfs2_stack_lock); >
On Tue, May 11, 2021 at 08:54:53PM +0800, Joseph Qi wrote: > > > On 5/11/21 3:16 PM, Dan Carpenter wrote: > > The snprintf() function returns the number of bytes which would have > > been printed if the buffer was large enough. In other words it can > > return ">= remain" but this code assumes it returns "== remain". > > > > The run time impact of this bug is not very severe. The next iteration > > through the loop would trigger a WARN() when we pass a negative limit > > to snprintf(). We would then return success instead of -E2BIG. > > > > The kernel implementation of snprintf() will never return negatives so > > there is no need to check and I have deleted that dead code. > > > > Fixes: a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Looks good. But the last 2 sections are introduced by: > 74ae4e104dfc ocfs2: Create stack glue sysfs files. > > With 'Fixes' tag updated, > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > Thanks! Will do. regards, dan carpenter
diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c index 90b8d300c1ee..de56e6231af8 100644 --- a/fs/ocfs2/filecheck.c +++ b/fs/ocfs2/filecheck.c @@ -326,11 +326,7 @@ static ssize_t ocfs2_filecheck_attr_show(struct kobject *kobj, ret = snprintf(buf + total, remain, "%lu\t\t%u\t%s\n", p->fe_ino, p->fe_done, ocfs2_filecheck_error(p->fe_status)); - if (ret < 0) { - total = ret; - break; - } - if (ret == remain) { + if (ret >= remain) { /* snprintf() didn't fit */ total = -E2BIG; break; diff --git a/fs/ocfs2/stackglue.c b/fs/ocfs2/stackglue.c index d50e8b8dfea4..16f1bfc407f2 100644 --- a/fs/ocfs2/stackglue.c +++ b/fs/ocfs2/stackglue.c @@ -500,11 +500,7 @@ static ssize_t ocfs2_loaded_cluster_plugins_show(struct kobject *kobj, list_for_each_entry(p, &ocfs2_stack_list, sp_list) { ret = snprintf(buf, remain, "%s\n", p->sp_name); - if (ret < 0) { - total = ret; - break; - } - if (ret == remain) { + if (ret >= remain) { /* snprintf() didn't fit */ total = -E2BIG; break; @@ -531,7 +527,7 @@ static ssize_t ocfs2_active_cluster_plugin_show(struct kobject *kobj, if (active_stack) { ret = snprintf(buf, PAGE_SIZE, "%s\n", active_stack->sp_name); - if (ret == PAGE_SIZE) + if (ret >= PAGE_SIZE) ret = -E2BIG; } spin_unlock(&ocfs2_stack_lock);
The snprintf() function returns the number of bytes which would have been printed if the buffer was large enough. In other words it can return ">= remain" but this code assumes it returns "== remain". The run time impact of this bug is not very severe. The next iteration through the loop would trigger a WARN() when we pass a negative limit to snprintf(). We would then return success instead of -E2BIG. The kernel implementation of snprintf() will never return negatives so there is no need to check and I have deleted that dead code. Fixes: a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/ocfs2/filecheck.c | 6 +----- fs/ocfs2/stackglue.c | 8 ++------ 2 files changed, 3 insertions(+), 11 deletions(-)