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

[Nathan Lynch]

"Serge E. Hallyn" <serge at hallyn.com> writes:
> Quoting Nathan Lynch (ntl at pobox.com):
>> "Serge E. Hallyn" <serge at hallyn.com> writes:
>> 
>> > Quoting Nathan Lynch (ntl at pobox.com):
>> >> "Serge E. Hallyn" <serue at us.ibm.com> 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 at us.ibm.com>
>> >> > ---
>> >> >  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.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers