cr: uts: don't pass an unsigned var as a signed int

PATCH In review Serge E. Hallyn 5 participants 11 messages Jun 19, 2009 – Jun 24, 2009 7 messages not in the archive

lore

Share & tools

lore

Serge E. Hallyn

author

2009-06-19 20:37 UTC #0 lore raw

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

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/[email protected]>

---
 checkpoint/namespace.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

checkpoint/namespace.c +6 -6

index 5726acb..8206aee 100644
@@ -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;

Signature · 1 line

1.6.1

A message that is not in the archive (sent off-list or not yet received): 20090619203719.GA30093-r/[email protected]

Nathan Lynch

2009-06-20 04:27 UTC #2 lore raw

Quoted 7 lines · 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/[email protected]>
---
 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.)

Quoted 33 lines

checkpoint/namespace.c +6 -6

index 5726acb..8206aee 100644
@@ -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;

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

2009-06-21 00:18 UTC #4 lore raw

Quoted 12 lines · Nathan Lynch

Quoted 7 lines · 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/[email protected]>
---
 checkpoint/namespace.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
But right above the code you're changing we have:

Quoted 8 lines

	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.

Quoted 2 lines

(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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Nathan Lynch

2009-06-21 05:29 UTC #6 lore raw

Quoted 20 lines · Serge E. Hallyn

Quoted 19 lines · Nathan Lynch
Quoted 7 lines · 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/[email protected]>
---
 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);

Quoted 3 lines

Quoted 1 line
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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

2009-06-21 13:53 UTC #8 lore raw

Quoted 48 lines · Nathan Lynch

Quoted 20 lines · Serge E. Hallyn
Quoted 19 lines · Nathan Lynch
Quoted 7 lines · 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/[email protected]>
---
 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);
Quoted 3 lines
Quoted 1 line
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);

Quoted 3 lines

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

Serge E. Hallyn

2009-06-21 19:13 UTC #9 lore raw

In reply to a message not in the archive

Quoted 48 lines · Nathan Lynch

Quoted 20 lines · Serge E. Hallyn
Quoted 19 lines · Nathan Lynch
Quoted 7 lines · 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/[email protected]>
---
 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);
Quoted 3 lines
Quoted 1 line
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);

Quoted 3 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Nathan Lynch

2009-06-22 04:13 UTC #11 lore raw

Quoted 53 lines · Serge E. Hallyn

Quoted 48 lines · Nathan Lynch
Quoted 20 lines · Serge E. Hallyn
Quoted 19 lines · Nathan Lynch
Quoted 7 lines · 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/[email protected]>
---
 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);
Quoted 3 lines
Quoted 1 line
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);
Quoted 3 lines
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.

Quoted 6 lines

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.

Quoted 4 lines

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oren Laadan

2009-06-24 17:07 UTC #13 lore raw

Nathan Lynch wrote:

[...]

Quoted 1 line

"Serge E. Hallyn" <[email protected]> writes:

[...]

Quoted 20 lines

Quoted 6 lines
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)

Quoted 12 lines

*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.
Quoted 4 lines
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.

Cedric Le Goater

2009-06-23 11:41 UTC #14 lore raw

In reply to a message not in the archive

Quoted 4 lines

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,

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

author

2009-06-23 13:05 UTC #16 lore raw

Quoted 5 lines · Cedric Le Goater

Quoted 4 lines
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?

Quoted 7 lines

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

Nathan Lynch

2009-06-22 23:20 UTC #17 lore raw

In reply to a message not in the archive

Quoted 31 lines · 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/[email protected]>

---
 checkpoint/namespace.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

checkpoint/namespace.c +4 -4

index 5726acb..8206aee 100644
@@ -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)

checkpoint/namespace.c static int do_checkpoint_uts_ns(struct ckpt_ctx *ctx, · line 67

Quoted 10 lines

checkpoint/namespace.c +2 -2

 		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?