[PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Serge E. Hallyn]

Else my checkpoing image gets reeeaallly huge.  Just passing the
result of sizeof() however does the right thing.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/namespace.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/checkpoint/namespace.c b/checkpoint/namespace.c
index 5726acb..8206aee 100644
--- a/checkpoint/namespace.c
+++ b/checkpoint/namespace.c
@@ -46,22 +46,22 @@ static int do_checkpoint_uts_ns(struct ckpt_ctx *ctx,
 		return ret;
 
 	down_read(&uts_sem);
-	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
+	ret = ckpt_write_string(ctx, name->sysname, sizeof(name->sysname));
 	if (ret < 0)
 		goto up;
-	ret = ckpt_write_string(ctx, name->nodename, h->nodename_len);
+	ret = ckpt_write_string(ctx, name->nodename, sizeof(name->nodename));
 	if (ret < 0)
 		goto up;
-	ret = ckpt_write_string(ctx, name->release, h->release_len);
+	ret = ckpt_write_string(ctx, name->release, sizeof(name->release));
 	if (ret < 0)
 		goto up;
-	ret = ckpt_write_string(ctx, name->version, h->version_len);
+	ret = ckpt_write_string(ctx, name->version, sizeof(name->version));
 	if (ret < 0)
 		goto up;
-	ret = ckpt_write_string(ctx, name->machine, h->machine_len);
+	ret = ckpt_write_string(ctx, name->machine, sizeof(name->machine));
 	if (ret < 0)
 		goto up;
-	ret = ckpt_write_string(ctx, name->domainname, h->domainname_len);
+	ret = ckpt_write_string(ctx, name->domainname, sizeof(name->domainname));
  up:
 	up_read(&uts_sem);
 	return ret;
-- 
1.6.1

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Nathan Lynch]

"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:

> Else my checkpoing image gets reeeaallly huge.  Just passing the
> result of sizeof() however does the right thing.
>
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/namespace.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)

But right above the code you're changing we have:

	h->sysname_len = sizeof(name->sysname);
	h->nodename_len = sizeof(name->nodename);
	h->release_len = sizeof(name->release);
	h->version_len = sizeof(name->version);
	h->machine_len = sizeof(name->machine);
	h->domainname_len = sizeof(name->domainname);

Your patch shouldn't change any behavior.  What gives?

(PS: ckpt_write_string and friends should take a size_t or some other
unsigned type for len.)


> diff --git a/checkpoint/namespace.c b/checkpoint/namespace.c
> index 5726acb..8206aee 100644
> --- a/checkpoint/namespace.c
> +++ b/checkpoint/namespace.c
> @@ -46,22 +46,22 @@ static int do_checkpoint_uts_ns(struct ckpt_ctx *ctx,
>  		return ret;
>  
>  	down_read(&uts_sem);
> -	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
> +	ret = ckpt_write_string(ctx, name->sysname, sizeof(name->sysname));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->nodename, h->nodename_len);
> +	ret = ckpt_write_string(ctx, name->nodename, sizeof(name->nodename));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->release, h->release_len);
> +	ret = ckpt_write_string(ctx, name->release, sizeof(name->release));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->version, h->version_len);
> +	ret = ckpt_write_string(ctx, name->version, sizeof(name->version));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->machine, h->machine_len);
> +	ret = ckpt_write_string(ctx, name->machine, sizeof(name->machine));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->domainname, h->domainname_len);
> +	ret = ckpt_write_string(ctx, name->domainname, sizeof(name->domainname));
>   up:
>  	up_read(&uts_sem);
>  	return ret;

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Else my checkpoing image gets reeeaallly huge.  Just passing the
> > result of sizeof() however does the right thing.
> >
> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > ---
> >  checkpoint/namespace.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> But right above the code you're changing we have:
> 
> 	h->sysname_len = sizeof(name->sysname);
> 	h->nodename_len = sizeof(name->nodename);
> 	h->release_len = sizeof(name->release);
> 	h->version_len = sizeof(name->version);
> 	h->machine_len = sizeof(name->machine);
> 	h->domainname_len = sizeof(name->domainname);
> 
> Your patch shouldn't change any behavior.  What gives?

"Shouldn't", perhaps, but does.

I assumed the compiler guessed that i wanted an int in the
second case and gave me a different result for that sizeof.

> (PS: ckpt_write_string and friends should take a size_t or some other
> unsigned type for len.)

There I agree, but then we need to check that no callers are
passing in potentially negative signed ints coming from userspace,
so I thought this patch would spur discussion about best path
forward (plus make checkpoint work for me again :)

-serge

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Nathan Lynch]

"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:

> Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
>> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
>> 
>> > Else my checkpoing image gets reeeaallly huge.  Just passing the
>> > result of sizeof() however does the right thing.
>> >
>> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  checkpoint/namespace.c |   12 ++++++------
>> >  1 files changed, 6 insertions(+), 6 deletions(-)
>> 
>> But right above the code you're changing we have:
>> 
>> 	h->sysname_len = sizeof(name->sysname);
>> 	h->nodename_len = sizeof(name->nodename);
>> 	h->release_len = sizeof(name->release);
>> 	h->version_len = sizeof(name->version);
>> 	h->machine_len = sizeof(name->machine);
>> 	h->domainname_len = sizeof(name->domainname);
>> 
>> Your patch shouldn't change any behavior.  What gives?
>
> "Shouldn't", perhaps, but does.


Revisiting do_checkpoint_uts_ns, I think it's a case of use after free:

	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_UTS_NS);
	if (!h)
		return -ENOMEM;

	h->sysname_len = sizeof(name->sysname);
	h->nodename_len = sizeof(name->nodename);
	h->release_len = sizeof(name->release);
	h->version_len = sizeof(name->version);
	h->machine_len = sizeof(name->machine);
	h->domainname_len = sizeof(name->domainname);

	ret = ckpt_write_obj(ctx, &h->h);
	ckpt_hdr_put(ctx, h);
	if (ret < 0)
		return ret;

	down_read(&uts_sem);
	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);

We're continuing to use h's memory after it has been released by
ckpt_hdr_put.  Seems plausible that the poison values written by sl*b
debug would cause the len argument to be ridiculously large.

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> >> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> >> 
> >> > Else my checkpoing image gets reeeaallly huge.  Just passing the
> >> > result of sizeof() however does the right thing.
> >> >
> >> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  checkpoint/namespace.c |   12 ++++++------
> >> >  1 files changed, 6 insertions(+), 6 deletions(-)
> >> 
> >> But right above the code you're changing we have:
> >> 
> >> 	h->sysname_len = sizeof(name->sysname);
> >> 	h->nodename_len = sizeof(name->nodename);
> >> 	h->release_len = sizeof(name->release);
> >> 	h->version_len = sizeof(name->version);
> >> 	h->machine_len = sizeof(name->machine);
> >> 	h->domainname_len = sizeof(name->domainname);
> >> 
> >> Your patch shouldn't change any behavior.  What gives?
> >
> > "Shouldn't", perhaps, but does.
> 
> 
> Revisiting do_checkpoint_uts_ns, I think it's a case of use after free:
> 
> 	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_UTS_NS);
> 	if (!h)
> 		return -ENOMEM;
> 
> 	h->sysname_len = sizeof(name->sysname);
> 	h->nodename_len = sizeof(name->nodename);
> 	h->release_len = sizeof(name->release);
> 	h->version_len = sizeof(name->version);
> 	h->machine_len = sizeof(name->machine);
> 	h->domainname_len = sizeof(name->domainname);
> 
> 	ret = ckpt_write_obj(ctx, &h->h);
> 	ckpt_hdr_put(ctx, h);
> 	if (ret < 0)
> 		return ret;
> 
> 	down_read(&uts_sem);
> 	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
> 
> We're continuing to use h's memory after it has been released by
> ckpt_hdr_put.  Seems plausible that the poison values written by sl*b
> debug would cause the len argument to be ridiculously large.

Haha.  Can't believe I didn't see that!

Thanks.

-serge

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> >> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> >> 
> >> > Else my checkpoing image gets reeeaallly huge.  Just passing the
> >> > result of sizeof() however does the right thing.
> >> >
> >> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >> > ---
> >> >  checkpoint/namespace.c |   12 ++++++------
> >> >  1 files changed, 6 insertions(+), 6 deletions(-)
> >> 
> >> But right above the code you're changing we have:
> >> 
> >> 	h->sysname_len = sizeof(name->sysname);
> >> 	h->nodename_len = sizeof(name->nodename);
> >> 	h->release_len = sizeof(name->release);
> >> 	h->version_len = sizeof(name->version);
> >> 	h->machine_len = sizeof(name->machine);
> >> 	h->domainname_len = sizeof(name->domainname);
> >> 
> >> Your patch shouldn't change any behavior.  What gives?
> >
> > "Shouldn't", perhaps, but does.
> 
> 
> Revisiting do_checkpoint_uts_ns, I think it's a case of use after free:
> 
> 	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_UTS_NS);
> 	if (!h)
> 		return -ENOMEM;
> 
> 	h->sysname_len = sizeof(name->sysname);
> 	h->nodename_len = sizeof(name->nodename);
> 	h->release_len = sizeof(name->release);
> 	h->version_len = sizeof(name->version);
> 	h->machine_len = sizeof(name->machine);
> 	h->domainname_len = sizeof(name->domainname);
> 
> 	ret = ckpt_write_obj(ctx, &h->h);
> 	ckpt_hdr_put(ctx, h);
> 	if (ret < 0)
> 		return ret;
> 
> 	down_read(&uts_sem);
> 	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
> 
> We're continuing to use h's memory after it has been released by
> ckpt_hdr_put.  Seems plausible that the poison values written by sl*b
> debug would cause the len argument to be ridiculously large.

Oren,

would it be possible to put up a filter, either manual or
automatic, to send every patch that gets pushed on the current
ckpt git branch to the containers list, maybe with a [CKPT PUSH]
tag in the subject line?

I think it will foster much more review of every patch.  Right now
it feels like we just catch blatant bugs when they bite us too hard,
but I don't think many people are looking through 'git wc' every
day.

-serge

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Nathan Lynch]

"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
>> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
>> 
>> > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
>> >> "Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
>> >> 
>> >> > Else my checkpoing image gets reeeaallly huge.  Just passing the
>> >> > result of sizeof() however does the right thing.
>> >> >
>> >> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> >> > ---
>> >> >  checkpoint/namespace.c |   12 ++++++------
>> >> >  1 files changed, 6 insertions(+), 6 deletions(-)
>> >> 
>> >> But right above the code you're changing we have:
>> >> 
>> >> 	h->sysname_len = sizeof(name->sysname);
>> >> 	h->nodename_len = sizeof(name->nodename);
>> >> 	h->release_len = sizeof(name->release);
>> >> 	h->version_len = sizeof(name->version);
>> >> 	h->machine_len = sizeof(name->machine);
>> >> 	h->domainname_len = sizeof(name->domainname);
>> >> 
>> >> Your patch shouldn't change any behavior.  What gives?
>> >
>> > "Shouldn't", perhaps, but does.
>> 
>> 
>> Revisiting do_checkpoint_uts_ns, I think it's a case of use after free:
>> 
>> 	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_UTS_NS);
>> 	if (!h)
>> 		return -ENOMEM;
>> 
>> 	h->sysname_len = sizeof(name->sysname);
>> 	h->nodename_len = sizeof(name->nodename);
>> 	h->release_len = sizeof(name->release);
>> 	h->version_len = sizeof(name->version);
>> 	h->machine_len = sizeof(name->machine);
>> 	h->domainname_len = sizeof(name->domainname);
>> 
>> 	ret = ckpt_write_obj(ctx, &h->h);
>> 	ckpt_hdr_put(ctx, h);
>> 	if (ret < 0)
>> 		return ret;
>> 
>> 	down_read(&uts_sem);
>> 	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
>> 
>> We're continuing to use h's memory after it has been released by
>> ckpt_hdr_put.  Seems plausible that the poison values written by sl*b
>> debug would cause the len argument to be ridiculously large.
>
> Oren,
>
> would it be possible to put up a filter, either manual or
> automatic, to send every patch that gets pushed on the current
> ckpt git branch to the containers list, maybe with a [CKPT PUSH]
> tag in the subject line?

Or just post patches to the mailing list before committing them to
public branches on which others are basing work.

This seems like an opportune moment to point out the guidelines for
including a tree in linux-next:

all patches/commits in the tree/series must have been:

	posted to a relevant mailing list
	reviewed
	unit tested
	destined for the next merge window (or the current release)

*before* they are included.

(source: http://lkml.org/lkml/2009/6/20/6 )

If upstream inclusion is the ultimate goal, those are the standards
which apply.


> I think it will foster much more review of every patch.  Right now
> it feels like we just catch blatant bugs when they bite us too hard,
> but I don't think many people are looking through 'git wc' every
> day.

Right.

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Oren Laadan]

Nathan Lynch wrote:

[...]

> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:

[...]

>> Oren,
>>
>> would it be possible to put up a filter, either manual or
>> automatic, to send every patch that gets pushed on the current
>> ckpt git branch to the containers list, maybe with a [CKPT PUSH]
>> tag in the subject line?
> 
> Or just post patches to the mailing list before committing them to
> public branches on which others are basing work.
> 
> This seems like an opportune moment to point out the guidelines for
> including a tree in linux-next:
> 
> all patches/commits in the tree/series must have been:
> 
> 	posted to a relevant mailing list
> 	reviewed
> 	unit tested
> 	destined for the next merge window (or the current release)
> 
> *before* they are included.
> 
> (source: http://lkml.org/lkml/2009/6/20/6 )
> 
> If upstream inclusion is the ultimate goal, those are the standards
> which apply.
> 
> 
>> I think it will foster much more review of every patch.  Right now
>> it feels like we just catch blatant bugs when they bite us too hard,
>> but I don't think many people are looking through 'git wc' every
>> day.

Yes, I agree with both suggestions, and will do both.

FWIW, I decided to push less-tested code to avoid delay for a long
period due to travel plans. Posting them on the list would have
helped to improve them, of course.

Oren.

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Cedric Le Goater]

> would it be possible to put up a filter, either manual or
> automatic, to send every patch that gets pushed on the current
> ckpt git branch to the containers list, maybe with a [CKPT PUSH]
> tag in the subject line?

you might want to use the 'post-receive' hook  like it's done on lxc-devel@ :

http://sourceforge.net/mailarchive/forum.php?thread_name=E1MF7MN-00089i-Oz%40fxgxhf1.ch3.sourceforge.com&forum_name=lxc-devel

Cheers,

C.

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Serge E. Hallyn]

Quoting Cedric Le Goater (legoater-GANU6spQydw@public.gmane.org):
> > would it be possible to put up a filter, either manual or
> > automatic, to send every patch that gets pushed on the current
> > ckpt git branch to the containers list, maybe with a [CKPT PUSH]
> > tag in the subject line?
> 
> you might want to use the 'post-receive' hook  like it's done on lxc-devel@ :
> 
> http://sourceforge.net/mailarchive/forum.php?thread_name=E1MF7MN-00089i-Oz%40fxgxhf1.ch3.sourceforge.com&forum_name=lxc-devel
> 
> Cheers,
> 
> C.

Thanks, neat idea.

As Nathan pointed out, it would be better (in terms of assuring upstream
maintainers of quality of the tree) to demand an ack *before* pushing to
git.  But even so, a post-receive hook would still be nice.

-serge

Re: [PATCH 1/1] cr: uts: don't pass an unsigned var as a signed int

[Nathan Lynch]

"Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> writes:
> Else my checkpoing image gets reeeaallly huge.  Just passing the
> result of sizeof() however does the right thing.
>
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/namespace.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/checkpoint/namespace.c b/checkpoint/namespace.c
> index 5726acb..8206aee 100644
> --- a/checkpoint/namespace.c
> +++ b/checkpoint/namespace.c
> @@ -46,22 +46,22 @@ static int do_checkpoint_uts_ns(struct ckpt_ctx *ctx,
>  		return ret;
>  
>  	down_read(&uts_sem);
> -	ret = ckpt_write_string(ctx, name->sysname, h->sysname_len);
> +	ret = ckpt_write_string(ctx, name->sysname, sizeof(name->sysname));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->nodename, h->nodename_len);
> +	ret = ckpt_write_string(ctx, name->nodename, sizeof(name->nodename));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->release, h->release_len);
> +	ret = ckpt_write_string(ctx, name->release, sizeof(name->release));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->version, h->version_len);
> +	ret = ckpt_write_string(ctx, name->version, sizeof(name->version));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->machine, h->machine_len);
> +	ret = ckpt_write_string(ctx, name->machine, sizeof(name->machine));
>  	if (ret < 0)
>  		goto up;
> -	ret = ckpt_write_string(ctx, name->domainname, h->domainname_len);
> +	ret = ckpt_write_string(ctx, name->domainname, sizeof(name->domainname));
>   up:
>  	up_read(&uts_sem);
>  	return ret;

So, getting back to the actual patch, this change seems fine -- just the
commit subject and message should be updated, yes?