[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[Cui Bixuan]

The msqid is 0 when a new message queue is created by msgget()
on the new system. Then 'TEST(msgget(msg_q, MSG_RW))' succeed
unexpectedly.

Run 'msg_q = msgget(IPC_PRIVATE, MSG_RW)', we get:
	msgget(IPC_PRIVATE, 0600)               = 0
The msg_q is 0 when create it on the new system.Next:
	msgctl(0, IPC_RMID, 0)                  = 0
	msgget(IPC_PRIVATE, 0600)               = 32768

The msg_q is equal to IPC_PRIVATE which is defined at kernel,
So msgget() will create a new message queue again and return
success unexpectedly.

Signed-off-by: Cui Bixuan <cuibixuan@huawei.com>
---
 testcases/kernel/syscalls/ipc/msgctl/msgctl13.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
index e170d6b..721dd88 100644
--- a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
+++ b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
@@ -60,9 +60,11 @@ static void msgctl_verify(void)
 {
 	int msg_q;
 
-	msg_q = msgget(IPC_PRIVATE, MSG_RW);
-	if (msg_q == -1)
-		tst_brkm(TBROK, cleanup, "Can't create message queue");
+	do {
+		msg_q = msgget(IPC_PRIVATE, MSG_RW);
+		if (msg_q == -1)
+			tst_brkm(TBROK, cleanup, "Can't create message queue");
+	} while (!msg_q);
 
 	TEST(msgctl(msg_q, IPC_RMID, NULL));
 
-- 
1.8.3.4

[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[Cyril Hrubis]

Hi!
> The msqid is 0 when a new message queue is created by msgget()
> on the new system. Then 'TEST(msgget(msg_q, MSG_RW))' succeed
> unexpectedly.
> 
> Run 'msg_q = msgget(IPC_PRIVATE, MSG_RW)', we get:
> 	msgget(IPC_PRIVATE, 0600)               = 0
> The msg_q is 0 when create it on the new system.Next:
> 	msgctl(0, IPC_RMID, 0)                  = 0
> 	msgget(IPC_PRIVATE, 0600)               = 32768
> 
> The msg_q is equal to IPC_PRIVATE which is defined at kernel,
> So msgget() will create a new message queue again and return
> success unexpectedly.

I've been able to reproduce the failure, for me the test failed on first
attempt after machine reboot then continued to work fine as the ID
returned from kernel seems to start at 0 and increases slowly.

I guess that we do not see the failure in LTP since there are other
msgctl() testcases executed before it.

Also running the testcase about 7000 times wraps around the msgid and
makes the test fail (you have to remove the message queue with key 0 by
hand since the test leaves it on the system).

> Signed-off-by: Cui Bixuan <cuibixuan@huawei.com>
> ---
>  testcases/kernel/syscalls/ipc/msgctl/msgctl13.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
> index e170d6b..721dd88 100644
> --- a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
> +++ b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
> @@ -60,9 +60,11 @@ static void msgctl_verify(void)
>  {
>  	int msg_q;
>  
> -	msg_q = msgget(IPC_PRIVATE, MSG_RW);
> -	if (msg_q == -1)
> -		tst_brkm(TBROK, cleanup, "Can't create message queue");
> +	do {
> +		msg_q = msgget(IPC_PRIVATE, MSG_RW);
> +		if (msg_q == -1)
> +			tst_brkm(TBROK, cleanup, "Can't create message queue");
> +	} while (!msg_q);
>  
>  	TEST(msgctl(msg_q, IPC_RMID, NULL));

This change leaks memory (leaves the message queue with key = 0 on the
system) and relies on a fact that IPC_PRIVATE == 0 which may not be
necessarilly true. Can't we instead do IPC_STAT on the msg_q and expect
it to fail?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] syscalls/msgctl13: fix error when run on the new system

[Cui Bixuan]

The msqid is 0 when a new message queue is created by msgget()
on the new system. Then 'TEST(msgget(msg_q, MSG_RW))' succeed
unexpectedly.

Run 'msg_q = msgget(IPC_PRIVATE, MSG_RW)', we get:
	msgget(IPC_PRIVATE, 0600)               = 0
The msg_q is 0 when create it on the new system.Next:
	msgctl(0, IPC_RMID, 0)                  = 0
	msgget(IPC_PRIVATE, 0600)               = 32768

The msg_q is equal to IPC_PRIVATE which is defined at kernel,
So msgget() will create a new message queue again and return
success unexpectedly.

Use msgctl with IPC_STAT to check it.

Signed-off-by: Cui Bixuan <cuibixuan@huawei.com>
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/ipc/msgctl/msgctl13.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
index e170d6b..50d48d4 100644
--- a/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
+++ b/testcases/kernel/syscalls/ipc/msgctl/msgctl13.c
@@ -28,6 +28,8 @@

 char *TCID = "msgctl13";
 int TST_TOTAL = 1;
+static struct msqid_ds buf;
+
 static void msgctl_verify(void);

 int main(int argc, char *argv[])
@@ -72,8 +74,8 @@ static void msgctl_verify(void)
 		return;
 	}

-	TEST(msgget(msg_q, MSG_RW));
-	if (TEST_ERRNO == ENOENT)
+	TEST(msgctl(msg_q, IPC_STAT, &buf));
+	if (TEST_ERRNO == EINVAL)
 		tst_resm(TPASS, "msgctl() test IPC_RMID succeeded");
 	else
 		tst_resm(TFAIL, "msgctl() test IPC_RMID failed unexpectedly");
-- 
1.8.3.4

[LTP] [PATCH v2] syscalls/msgctl13: fix error when run on the new system

[Cyril Hrubis]

Hi!
I've reworded the commit description a bit and pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[Cui Bixuan]

On 2016/4/12 0:58, Cyril Hrubis wrote:
> This change leaks memory (leaves the message queue with key = 0 on the
> system) and relies on a fact that IPC_PRIVATE == 0 which may not be
> necessarilly true. Can't we instead do IPC_STAT on the msg_q and expect
> it to fail?
Use msgctl with IPC_STAT to check it is better :-);

But why it return EINVAL, instead of EIDRM?

Thanks
Cui Bixuan

> 
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[Cyril Hrubis]

Hi!
> But why it return EINVAL, instead of EIDRM?

Since at the point we do the IPC_STAT the ID is allready unused hence
it's not valid anymore. You get EIDRM only in case that process is
waiting in kernel (for example in msgrcv()) while another process
removes the queue.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/msgctl13: fix error when run on the new system

[Cui Bixuan]

On 2016/4/13 18:51, Cyril Hrubis wrote:
> Hi!
>> But why it return EINVAL, instead of EIDRM?
> 
> Since at the point we do the IPC_STAT the ID is allready unused hence
> it's not valid anymore. You get EIDRM only in case that process is
> waiting in kernel (for example in msgrcv()) while another process
> removes the queue.
Get it, Thank you.
>