Skip to contentSkip to conversation

ocfs2: fix snprintf() checking

PATCH Superseded Dan Carpenter 2 participants 4 messages May 11, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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.

Fixes a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check")
---
 fs/ocfs2/filecheck.c | 6 +-----
 fs/ocfs2/stackglue.c | 8 ++------
 2 files changed, 3 insertions(+), 11 deletions(-)
fs/ocfs2/filecheck.c +1 -5
index 90b8d300c1ee..de56e6231af8 100644
@@ -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
index d50e8b8dfea4..16f1bfc407f2 100644
@@ -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
maintainer Reviewed-by on v1 1/1
#1 lore raw
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 to snprintf(). 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")

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
index 90b8d300c1ee..de56e6231af8 100644
@@ -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
index d50e8b8dfea4..16f1bfc407f2 100644
@@ -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);
#2 lore raw
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 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.

Quoted 9 lines
Quoted 2 lines
Fixes a860f6eb4c6a ("ocfs2: sysfile interfaces for online file check")

Looks good. But the last 2 sections are introduced by:
74ae4e104dfc ocfs2: Create stack glue sysfs files.

With 'Fixes' tag updated,

Thanks! Will do.

regards,
dan carpenter

#3 lore raw

PATCH v2 1/1 ocfs2: fix snprintf() checking

In reply to Joseph Qi

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")
Fixes 74ae4e104dfc ("ocfs2: Create stack glue sysfs files.")

---
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
index 90b8d300c1ee..de56e6231af8 100644
@@ -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
index d50e8b8dfea4..16f1bfc407f2 100644
@@ -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