Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps(): don't rely onundefined behaviour

From: Jesper Juhl
Date: Thu Mar 22 2012 - 19:18:10 EST

Next message: Jesper Juhl: "And patch 2 as well - Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps():don't rely on undefined behaviour"
Previous message: Andrew Morton: "Re: [PATCH] proc-ns: Use d_set_d_op() API to set dentry ops inproc_ns_instantiate()."
In reply to: Jesper Juhl: "Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps(): don't rely onundefined behaviour"
Next in thread: Jesper Juhl: "And patch 2 as well - Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps():don't rely on undefined behaviour"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 23 Mar 2012, Jesper Juhl wrote:

> On Tue, 20 Mar 2012, Greg KH wrote:
>
> > On Tue, Mar 20, 2012 at 12:40:50AM +0100, Jesper Juhl wrote:
> > > On Mon, 19 Mar 2012, Joe Perches wrote:
> > >
> > > > On Tue, 2012-03-20 at 00:19 +0100, Jesper Juhl wrote:
> > > > > On Mon, 19 Mar 2012, Joe Perches wrote:
> > > > > > On Mon, 2012-03-19 at 23:46 +0100, Jesper Juhl wrote:
> > > > > > > On Mon, 19 Mar 2012, Joe Perches wrote:
> > > > > > > > On Mon, 2012-03-19 at 23:37 +0100, Jesper Juhl wrote:
> > > > > > > > > In drivers/telephony/ixj.c::add_caps() we have several statements like this:
> > > > > > > > > j->caplist[j->caps].handle = j->caps++;
> > > > > > > > > That's undefined behaviour right there.
> > > > > > > > telephony has been moved to staging.
> > > > > > > Since when? Where?
> > > > > > > In my up-to-date Linus tree with HEAD at
> > > > > > > c16fa4f2ad19908a47c63d8fa436a1178438c7e7, that file is is still in
> > > > > > > drivers/telephony/, not in staging/...
> > > > > > > /confused
> > > > > > In the -next tree.
> > > > > Ok, seems I've missed that.
> > > > > > Yes, it's a bug fix, but drivers/telephony is pretty dead.
> > > > > Dead or not, as long as it's in the tree I think that fixing bugs is
> > > > > relevant.
> > > > > Besides, who knows if/when it'll get ressurrected ;)
> > > >
> > > > Sorry, I didn't mean to suggest it shouldn't
> > > > be fixed.
> > > >
> > > > I meant that it probably didn't need to be
> > > > fixed during the merge window or maybe even
> > > > not backported to stable unless you're sure
> > > > the order of operations is now done correctly
> > > > and with no real change in current operation
> > > > by inspecting the object. I presume it worked
> > > > before but it's likely not too many people
> > > > actually still use this hardware with the
> > > > current kernel.
> > > >
> > > I never intended to push it for -stable, it probably "works" in its
> > > current form with any relevant compiler. I just spotted a bug and wanted
> > > to fix it :-)
> > > If it gets fixed during the merge window or at some other time I don't
> > > really care - but I don't see any reason to not just fix it as soon as
> > > possible.
> > >
> > > I have no idea how many people still use this hardware with current
> > > kernels, but even if just a few do, they deserve to get code that has
> > > well-defined behaviour with standards conforming C compilers and not what
> > > is currently there that can change with different compilers/different
> > > compiler versions.
> >
> > Then care to send me the patch, against the linux-next tree, so I can
> > queue it up for the 3.5 merge window?
> >
> Sure thing. Here's one against next-20120322
>
>
>
> >From e518aca86e08129b71204c07200ec450a6fbe2c7 Mon Sep 17 00:00:00 2001
> From: Jesper Juhl <jj@xxxxxxxxxxxxxx>

Whoops, that should have been:

From: Jesper Juhl <jj@xxxxxxxxxxxxx>

And I forgot a

Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>

> Date: Thu, 22 Mar 2012 17:54:58 +0100
> Subject: [PATCH 1/2] drivers/staging/telephony/ixj.c::add_caps(): don't rely
> on undefined behaviour
>
> In drivers/staging/telephony/ixj.c::add_caps() we have several statements like this:
>
> j->caplist[j->caps].handle = j->caps++;
>
> That's undefined behaviour right there. Since j->caps is both read
> (left hand side of assignment) and written to (right hand side of
> assignment) but there is no intervening sequence point, the order
> of the read and write happening is undefined.
>
> I believe that what we want to have happen is that the assignment
> happens with the same value of "j->caps" that's used as the
> left-hand-side index value and then subsequently increment
> "j->caps". So I've changed all those statements to:
>
> j->caplist[j->caps].handle = j->caps;
> j->caps++;
>
> which has well-defined behaviour since there's now a sequence point
> (";") involved, so we know the order of the read/write.
> ---
> drivers/staging/telephony/ixj.c | 57 ++++++++++++++++++++++++++-------------
> 1 file changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/telephony/ixj.c b/drivers/staging/telephony/ixj.c
> index d5f923b..f960279 100644
> --- a/drivers/staging/telephony/ixj.c
> +++ b/drivers/staging/telephony/ixj.c
> @@ -5927,7 +5927,8 @@ static void add_caps(IXJ *j)
> j->caplist[j->caps].cap = PHONE_VENDOR_QUICKNET;
> strcpy(j->caplist[j->caps].desc, "Quicknet Technologies, Inc. (www.quicknet.net)");
> j->caplist[j->caps].captype = vendor;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> j->caplist[j->caps].captype = device;
> switch (j->cardtype) {
> case QTI_PHONEJACK:
> @@ -5947,11 +5948,13 @@ static void add_caps(IXJ *j)
> break;
> }
> j->caplist[j->caps].cap = j->cardtype;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> strcpy(j->caplist[j->caps].desc, "POTS");
> j->caplist[j->caps].captype = port;
> j->caplist[j->caps].cap = pots;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> /* add devices that can do speaker/mic */
> switch (j->cardtype) {
> @@ -5962,7 +5965,8 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "SPEAKER");
> j->caplist[j->caps].captype = port;
> j->caplist[j->caps].cap = speaker;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> default:
> break;
> }
> @@ -5973,7 +5977,8 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "HANDSET");
> j->caplist[j->caps].captype = port;
> j->caplist[j->caps].cap = handset;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> break;
> default:
> break;
> @@ -5985,7 +5990,8 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "PSTN");
> j->caplist[j->caps].captype = port;
> j->caplist[j->caps].cap = pstn;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> break;
> default:
> break;
> @@ -5995,50 +6001,59 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "ULAW");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = ULAW;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "LINEAR 16 bit");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = LINEAR16;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "LINEAR 8 bit");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = LINEAR8;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "Windows Sound System");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = WSS;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> /* software ALAW codec, made from ULAW */
> strcpy(j->caplist[j->caps].desc, "ALAW");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = ALAW;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> /* version 12 of the 8020 does the following codecs in a broken way */
> if (j->dsp.low != 0x20 || j->ver.low != 0x12) {
> strcpy(j->caplist[j->caps].desc, "G.723.1 6.3kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = G723_63;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "G.723.1 5.3kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = G723_53;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "TrueSpeech 4.8kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = TS48;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
>
> strcpy(j->caplist[j->caps].desc, "TrueSpeech 4.1kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = TS41;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> }
>
> /* 8020 chips can do TS8.5 native, and 8021/8022 can load it */
> @@ -6046,7 +6061,8 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "TrueSpeech 8.5kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = TS85;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> }
>
> /* 8021 chips can do G728 */
> @@ -6054,7 +6070,8 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "G.728 16kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = G728;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> }
>
> /* 8021/8022 chips can do G729 if loaded */
> @@ -6062,13 +6079,15 @@ static void add_caps(IXJ *j)
> strcpy(j->caplist[j->caps].desc, "G.729A 8kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = G729;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> }
> if (j->dsp.low != 0x20 && j->flags.g729_loaded) {
> strcpy(j->caplist[j->caps].desc, "G.729B 8kbps");
> j->caplist[j->caps].captype = codec;
> j->caplist[j->caps].cap = G729B;
> - j->caplist[j->caps].handle = j->caps++;
> + j->caplist[j->caps].handle = j->caps;
> + j->caps++;
> }
> }
>
>

--
Jesper Juhl <jj@xxxxxxxxxxxxx> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

--
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/

Next message: Jesper Juhl: "And patch 2 as well - Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps():don't rely on undefined behaviour"
Previous message: Andrew Morton: "Re: [PATCH] proc-ns: Use d_set_d_op() API to set dentry ops inproc_ns_instantiate()."
In reply to: Jesper Juhl: "Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps(): don't rely onundefined behaviour"
Next in thread: Jesper Juhl: "And patch 2 as well - Re: [PATCH 1/2] drivers/telephony/ixj.c::add_caps():don't rely on undefined behaviour"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]