Re: [PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

On Mon, Oct 19, 2009 at 6:29 PM, Jiri Kosina <jkosina@xxxxxxx> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> >> >> ipc/msg.c: In function ?msgctl_down?:
>> >> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>> >> >>
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
>> >> >> ---
>> >> >> Âipc/msg.c | Â Â2 +-
>> >> >> Â1 files changed, 1 insertions(+), 1 deletions(-)
>> >> >>
>> >> >> diff --git a/ipc/msg.c b/ipc/msg.c
>> >> >> index 2ceab7f..085bd58 100644
>> >> >> --- a/ipc/msg.c
>> >> >> +++ b/ipc/msg.c
>> >> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> >> >> Â Â Â Â Â Â Â Â Â Â Âstruct msqid_ds __user *buf, int version)
>> >> >> Â{
>> >> >> Â Â Â struct kern_ipc_perm *ipcp;
>> >> >> - Â Â struct msqid64_ds msqid64;
>> >> >> + Â Â struct msqid64_ds uninitialized_var(msqid64);
>> >> >> Â Â Â struct msg_queue *msq;
>> >> >> Â Â Â int err;
> [ ... snip ... ]
>> > I have verified both with 4.1 and 4.3, and it doesn't emit this
>> > false-positive warning, so there have been gcc versions getting this
>> > right. Ergo gcc developers should rather fix this "regression" and revert
>> > to the old behavior, methinks.
>>
>> The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
>> finds an actual problem in the code. I will try to dig deeper to see
>> what's actually happening... at first glance I don't see who is
>> initializing msqid64.
>
> This statement of your makes me wonder why you have submitted the patch in
> the first place, as you are apparently not sure whether adding
> uninitialized_var() is a valid thing to do or not.

Because I want to get rid of the warning?

Also, if you look at the commit I mentioned in the commit message
(a0d092f), you'll see this change:
-	struct msq_setbuf uninitialized_var(setbuf);
+	struct msq_setbuf setbuf;

Which is ok, because in that version msgctl_down is calling
audit_ipc_set_perm, directly, but only when cmd == IPC_SET. However,
later on (a5f75e7), ipcctl_pre_down was introduced, and the check was
delegated, and that most probably introduced the warning.

In any case, if the patch is not correct, then somebody should point
that out, which is what the patch review process is for. Alternatively
I could have sent an email asking what is happening here, but from my
point of view this patch serves exactly the same purpose, and has the
advantage that it might turn out to be correct.

It's not as if I didn't do any homework while writing this patch.

> The gcc warning in this case is actually bogus, as msqid64 is touched only
> iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes
> it properly.

Yes, but it's used by ipcctl_pre_down, which from what I can see is
only using those arguments when cmd == IPC_SET, so everything is ok.
But still, I don't think the compiler _should_ know what
ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
then uninitialized_var is OK.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/