ocfs2: fix snprintf() checking
Subsystems (2)
PATCH 1/1 ocfs2: fix snprintf() checking
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.
--- fs/ocfs2/filecheck.c | 6 +----- fs/ocfs2/stackglue.c | 8 ++------ 2 files changed, 3 insertions(+), 11 deletions(-)
fs/ocfs2/filecheck.c +1 -5
@@ -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 ;
fs/ocfs2/stackglue.c +2 -6
@@ -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);
Signature · 1 line
2.30.2
Quoted 8 lines · Dan Carpenter
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 tosnprintf(). We would then return success instead of-E2BIG.
Quoted 5 lines
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 <[email protected]>
Looks good. But the last 2 sections are introduced by:74ae4e104dfc ocfs2: Create stack glue sysfs files.
With 'Fixes' tag updated,
Quoted 49 lines
--- fs/ocfs2/filecheck.c | 6 +----- fs/ocfs2/stackglue.c | 8 ++------ 2 files changed, 3 insertions(+), 11 deletions(-)fs/ocfs2/filecheck.c +1
-5@@ -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 ;fs/ocfs2/stackglue.c +2
-6@@ -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);
Quoted 14 lines · Joseph Qi
Quoted 11 lines · Dan Carpenter
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 tosnprintf(). 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.
Quoted 9 lines
Quoted 2 lines
Fixes a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check")Signed-off-by Dan Carpenter <[email protected]>Looks good. But the last 2 sections are introduced by:
74ae4e104dfcocfs2: Create stack glue sysfs files.With 'Fixes' tag updated,
Reviewed-by Joseph Qi <[email protected]>
Thanks! Will do.
regards,
dan carpenter
PATCH v2 1/1 ocfs2: fix snprintf() checking
In reply to Joseph QiThe 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.
---
v2: update the Fixes tag, add Joseph's R-b tag.
fs/ocfs2/filecheck.c | 6 +----- fs/ocfs2/stackglue.c | 8 ++------ 2 files changed, 3 insertions(+), 11 deletions(-)
fs/ocfs2/filecheck.c +1 -5
@@ -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 ;
fs/ocfs2/stackglue.c +2 -6
@@ -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);
Signature · 1 line
2.30.2