[PATCH] Properly handle \0 delimited string lists

[PATCH] Properly handle \0 delimited string lists

[Jack Miller]

reserved-names="res1\0res2\0res3";

Is valid DTS. This one-liner expands data based on the len given by the lexer
instead of strlen.

Without this patch, realloc gets confused and hangs. For example:

*** Error in `./dtc': realloc(): invalid next size: 0x0000000001961670
***
---
 data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/data.c b/data.c
index 4c50b12..8cae237 100644
--- a/data.c
+++ b/data.c
@@ -74,7 +74,7 @@ struct data data_copy_escape_string(const char *s, int len)
 	struct data d;
 	char *q;
 
-	d = data_grow_for(empty_data, strlen(s)+1);
+	d = data_grow_for(empty_data, len + 1);
 
 	q = d.val;
 	while (i < len) {
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Properly handle \0 delimited string lists

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

Hi Jack,

Long time no see :).

On Tue, Jun 10, 2014 at 02:57:36PM -0500, Jack Miller wrote:
> reserved-names="res1\0res2\0res3";
> 
> Is valid DTS. This one-liner expands data based on the len given by the lexer
> instead of strlen.
> 
> Without this patch, realloc gets confused and hangs. For example:
> 
> *** Error in `./dtc': realloc(): invalid next size: 0x0000000001961670
> ***

So.. the patch certainly isn't wrong, and is arguably safer than the
current version.

But.. I haven't been able to reproduce the problem, and I don't really
see how it would occur in the first place.

The thing we're taking a strlen of is the input with it's escapes, so
it won't have NULs, just backslashes and 0 digits.

Or am I missing something?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Properly handle \0 delimited string lists

[Jack Miller]

On Wed, Jun 11, 2014 at 11:10:39PM +1000, David Gibson wrote:
> Hi Jack,
> 
> Long time no see :).

Yeah, I was pretty pleased to have an excuse to send something your way =)

> 
> On Tue, Jun 10, 2014 at 02:57:36PM -0500, Jack Miller wrote:
> > reserved-names="res1\0res2\0res3";
> > 
> > Is valid DTS. This one-liner expands data based on the len given by the lexer
> > instead of strlen.
> > 
> > Without this patch, realloc gets confused and hangs. For example:
> > 
> > *** Error in `./dtc': realloc(): invalid next size: 0x0000000001961670
> > ***
> 
> So.. the patch certainly isn't wrong, and is arguably safer than the
> current version.
> 
> But.. I haven't been able to reproduce the problem, and I don't really
> see how it would occur in the first place.
> 
> The thing we're taking a strlen of is the input with it's escapes, so
> it won't have NULs, just backslashes and 0 digits.
> 
> Or am I missing something?

Sorry, I was unclear. The \0 was my short hand for a real embedded NULL
character, which may be intentionally wrong-headed, but I don't think it's
invalid (or if it is invalid, should at least not cause the compiler to do
bad things).

In refining my testcase I realized that it doesn't fail on realloc with all
bad input, but it does generate mangled output otherwise.

I've uploaded two short .dts snippets just because pasting NULLs into an
email seems like a bad idea:

http://codezen.org/static/broken-dts.tar.gz

One causes the realloc, the other causes the mangled output on git HEAD. Both
are working properly with my patch.

- Jack
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Properly handle \0 delimited string lists

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

On Thu, Jun 12, 2014 at 05:47:28PM -0500, Jack Miller wrote:
> On Wed, Jun 11, 2014 at 11:10:39PM +1000, David Gibson wrote:
> > Hi Jack,
> > 
> > Long time no see :).
> 
> Yeah, I was pretty pleased to have an excuse to send something your way =)
> 
> > 
> > On Tue, Jun 10, 2014 at 02:57:36PM -0500, Jack Miller wrote:
> > > reserved-names="res1\0res2\0res3";
> > > 
> > > Is valid DTS. This one-liner expands data based on the len given by the lexer
> > > instead of strlen.
> > > 
> > > Without this patch, realloc gets confused and hangs. For example:
> > > 
> > > *** Error in `./dtc': realloc(): invalid next size: 0x0000000001961670
> > > ***
> > 
> > So.. the patch certainly isn't wrong, and is arguably safer than the
> > current version.
> > 
> > But.. I haven't been able to reproduce the problem, and I don't really
> > see how it would occur in the first place.
> > 
> > The thing we're taking a strlen of is the input with it's escapes, so
> > it won't have NULs, just backslashes and 0 digits.
> > 
> > Or am I missing something?
> 
> Sorry, I was unclear. The \0 was my short hand for a real embedded NULL
> character, which may be intentionally wrong-headed, but I don't think it's
> invalid (or if it is invalid, should at least not cause the compiler to do
> bad things).

Ah, I see.

Yes, NULs in the input is a bit perverse, but I don't see any reason
it should be invalid.

> In refining my testcase I realized that it doesn't fail on realloc with all
> bad input, but it does generate mangled output otherwise.
> 
> I've uploaded two short .dts snippets just because pasting NULLs into an
> email seems like a bad idea:
> 
> http://codezen.org/static/broken-dts.tar.gz
> 
> One causes the realloc, the other causes the mangled output on git HEAD. Both
> are working properly with my patch.

Ok.  Could you make those into a testcase?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]