| Message ID | 20250220220758.GT21808@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs_scrub: fix buffer overflow in string_escape | expand |
On 2025-02-20 14:07:58, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Need to allocate one more byte for the null terminator, just in case the > /entire/ input string consists of non-printable bytes e.g. emoji. > > Cc: <linux-xfs@vger.kernel.org> # v4.15.0 > Fixes: 396cd0223598bb ("xfs_scrub: warn about suspicious characters in directory/xattr names") > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org> > --- > scrub/common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scrub/common.c b/scrub/common.c > index 6eb3c026dc5ac9..7ea0277bc511ce 100644 > --- a/scrub/common.c > +++ b/scrub/common.c > @@ -320,7 +320,7 @@ string_escape( > char *q; > int x; > > - str = malloc(strlen(in) * 4); > + str = malloc((strlen(in) * 4) + 1); > if (!str) > return NULL; > for (p = in, q = str; *p != '\0'; p++) { > Looks good to me Reviewed-by: Andrey Albershteyn <aalbersh@kernel.org>
On Thu, Feb 20, 2025 at 02:07:58PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Need to allocate one more byte for the null terminator, just in case the > /entire/ input string consists of non-printable bytes e.g. emoji. > > Cc: <linux-xfs@vger.kernel.org> # v4.15.0 > Fixes: 396cd0223598bb ("xfs_scrub: warn about suspicious characters in directory/xattr names") > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org> > --- > scrub/common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scrub/common.c b/scrub/common.c > index 6eb3c026dc5ac9..7ea0277bc511ce 100644 > --- a/scrub/common.c > +++ b/scrub/common.c > @@ -320,7 +320,7 @@ string_escape( > char *q; > int x; > > - str = malloc(strlen(in) * 4); > + str = malloc((strlen(in) * 4) + 1); Nit: no need for the inner braces. But this open code string allocation and manipulation makes me feel really bad. Assuming we don't have a good alternative, can you at least throw in a comment explaining the allocation length here?
On Mon, Feb 24, 2025 at 03:11:03PM +0100, Christoph Hellwig wrote: > On Thu, Feb 20, 2025 at 02:07:58PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Need to allocate one more byte for the null terminator, just in case the > > /entire/ input string consists of non-printable bytes e.g. emoji. > > > > Cc: <linux-xfs@vger.kernel.org> # v4.15.0 > > Fixes: 396cd0223598bb ("xfs_scrub: warn about suspicious characters in directory/xattr names") > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org> > > --- > > scrub/common.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/scrub/common.c b/scrub/common.c > > index 6eb3c026dc5ac9..7ea0277bc511ce 100644 > > --- a/scrub/common.c > > +++ b/scrub/common.c > > @@ -320,7 +320,7 @@ string_escape( > > char *q; > > int x; > > > > - str = malloc(strlen(in) * 4); > > + str = malloc((strlen(in) * 4) + 1); > > Nit: no need for the inner braces. > > But this open code string allocation and manipulation makes me feel > really bad. Assuming we don't have a good alternative, can you > at least throw in a comment explaining the allocation length here? Ok. I'll change the code to read as follows: /* * Each non-printing byte renders as a four-byte escape sequence, so * allocate 4x the input length, plus a byte for the null terminator. */ str = malloc(strlen(in) * 4 + 1); --D
diff --git a/scrub/common.c b/scrub/common.c index 6eb3c026dc5ac9..7ea0277bc511ce 100644 --- a/scrub/common.c +++ b/scrub/common.c @@ -320,7 +320,7 @@ string_escape( char *q; int x; - str = malloc(strlen(in) * 4); + str = malloc((strlen(in) * 4) + 1); if (!str) return NULL; for (p = in, q = str; *p != '\0'; p++) {