[RFC] fs:unicode:mkutf8data.c: Fix the potential stack overflow risk

[RFC] fs:unicode:mkutf8data.c: Fix the potential stack overflow risk

[jianchunfu]

I'm not sure why there are so many missing checks of the malloc function,
is it because the memory allocated is only a few bytes
so no checks are needed?

Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>
---
 fs/unicode/mkutf8data.c | 54 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/fs/unicode/mkutf8data.c b/fs/unicode/mkutf8data.c
index 8c2ace050..1f9e3ab1e 100644
--- a/fs/unicode/mkutf8data.c
+++ b/fs/unicode/mkutf8data.c
@@ -2164,6 +2164,10 @@ static void nfdi_init(void)
 		mapping[i++] = 0;
 
 		um = malloc(i * sizeof(unsigned int));
+		if (!um) {
+			printf("Memory allocation failed\n");
+			exit(1);
+		}
 		memcpy(um, mapping, i * sizeof(unsigned int));
 		unicode_data[unichar].utf32nfdi = um;
 
@@ -2220,6 +2224,10 @@ static void nfdicf_init(void)
 		mapping[i++] = 0;
 
 		um = malloc(i * sizeof(unsigned int));
+		if (!um) {
+			printf("Memory allocation failed\n");
+			exit(1);
+		}
 		memcpy(um, mapping, i * sizeof(unsigned int));
 		unicode_data[unichar].utf32nfdicf = um;
 
@@ -2261,10 +2269,18 @@ static void ignore_init(void)
 			for (unichar = first; unichar <= last; unichar++) {
 				free(unicode_data[unichar].utf32nfdi);
 				um = malloc(sizeof(unsigned int));
+				if (!um) {
+					ret = -ENOMEM;
+					goto error_nomem;
+				}
 				*um = 0;
 				unicode_data[unichar].utf32nfdi = um;
 				free(unicode_data[unichar].utf32nfdicf);
 				um = malloc(sizeof(unsigned int));
+				if (!um) {
+					ret = -ENOMEM;
+					goto error_nomem;
+				}
 				*um = 0;
 				unicode_data[unichar].utf32nfdicf = um;
 				count++;
@@ -2282,10 +2298,18 @@ static void ignore_init(void)
 				line_fail(prop_name, line);
 			free(unicode_data[unichar].utf32nfdi);
 			um = malloc(sizeof(unsigned int));
+			if (!um) {
+				ret = -ENOMEM;
+				goto error_nomem;
+			}
 			*um = 0;
 			unicode_data[unichar].utf32nfdi = um;
 			free(unicode_data[unichar].utf32nfdicf);
 			um = malloc(sizeof(unsigned int));
+			if (!um) {
+				ret = -ENOMEM;
+				goto error_nomem;
+			}
 			*um = 0;
 			unicode_data[unichar].utf32nfdicf = um;
 			if (verbose > 1)
@@ -2301,6 +2325,12 @@ static void ignore_init(void)
 		printf("Found %d entries\n", count);
 	if (count == 0)
 		file_fail(prop_name);
+
+error_nomem:
+	if (ret == -ENOMEM) {
+		printf("Memory allocation failed\n");
+		exit(1);
+	}
 }
 
 static void corrections_init(void)
@@ -2364,6 +2394,10 @@ static void corrections_init(void)
 		mapping[i++] = 0;
 
 		um = malloc(i * sizeof(unsigned int));
+		if (!um) {
+			printf("Memory allocation failed\n");
+			exit(1);
+		}
 		memcpy(um, mapping, i * sizeof(unsigned int));
 		corrections[count].utf32nfdi = um;
 
@@ -2464,11 +2498,19 @@ static void hangul_decompose(void)
 
 		assert(!unicode_data[unichar].utf32nfdi);
 		um = malloc(i * sizeof(unsigned int));
+		if (!um) {
+			printf("Memory allocation failed\n");
+			exit(1);
+		}
 		memcpy(um, mapping, i * sizeof(unsigned int));
 		unicode_data[unichar].utf32nfdi = um;
 
 		assert(!unicode_data[unichar].utf32nfdicf);
 		um = malloc(i * sizeof(unsigned int));
+		if (!um) {
+			printf("Memory allocation failed\n");
+			exit(1);
+		}
 		memcpy(um, mapping, i * sizeof(unsigned int));
 		unicode_data[unichar].utf32nfdicf = um;
 
@@ -2528,12 +2570,20 @@ static void nfdi_decompose(void)
 				break;
 			free(unicode_data[unichar].utf32nfdi);
 			um = malloc(i * sizeof(unsigned int));
+			if (!um) {
+				printf("Memory allocation failed\n");
+				exit(1);
+			}
 			memcpy(um, mapping, i * sizeof(unsigned int));
 			unicode_data[unichar].utf32nfdi = um;
 		}
 		/* Add this decomposition to nfdicf if there is no entry. */
 		if (!unicode_data[unichar].utf32nfdicf) {
 			um = malloc(i * sizeof(unsigned int));
+			if (!um) {
+				printf("Memory allocation failed\n");
+				exit(1);
+			}
 			memcpy(um, mapping, i * sizeof(unsigned int));
 			unicode_data[unichar].utf32nfdicf = um;
 		}
@@ -2582,6 +2632,10 @@ static void nfdicf_decompose(void)
 				break;
 			free(unicode_data[unichar].utf32nfdicf);
 			um = malloc(i * sizeof(unsigned int));
+			if (!um) {
+				printf("Memory allocation failed\n");
+				exit(1);
+			}
 			memcpy(um, mapping, i * sizeof(unsigned int));
 			unicode_data[unichar].utf32nfdicf = um;
 		}
-- 
2.18.4

Re: [RFC] fs:unicode:mkutf8data.c: Fix the potential stack overflow risk

[Eric Biggers]

On Fri, Mar 25, 2022 at 05:14:43PM +0800, jianchunfu wrote:
> I'm not sure why there are so many missing checks of the malloc function,
> is it because the memory allocated is only a few bytes
> so no checks are needed?
> 
> Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>
> ---
>  fs/unicode/mkutf8data.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 

mkutf8data is a host tool used during the build, not kernel code, so it doesn't
really matter.  If malloc returns NULL, the tool will crash, which will be
treated as a build error, just like if it cleanly reported a failure.  It's
definitely poor practice, though.

How about just adding and using a helper function "xmalloc()" that has a NULL
check built in?  That would be much simpler than your patch.

- Eric

Re: [RFC] fs:unicode:mkutf8data.c: Fix the potential stack overflow risk

[Gabriel Krisman Bertazi]

jianchunfu <jianchunfu@cmss.chinamobile.com> writes:

> I'm not sure why there are so many missing checks of the malloc function,
> is it because the memory allocated is only a few bytes
> so no checks are needed?
>
> Signed-off-by: jianchunfu <jianchunfu@cmss.chinamobile.com>

Hi jianchunfu,

Thanks for the patch.

Beyond what Eric said, the patch prefix should be just "unicode:".  When
in doubt you can see the previous patches to the subsystem in the git
log.  Also, I think these are not really  stack overflows, but a bad
memory access if malloc fails.  What do you think of something like

unicode: Handle memory allocation failures in mkutf8data

or something like that.

Thanks,

-- 
Gabriel Krisman Bertazi