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

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

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