Re: [HELP REQUESTED from the community] Was: Staging status of speakup

[Chris Brannon]

Gregory Nowak  writes:

> keymap
> I believe this is the currently active kernel keymap. I'm not sure of
> the format, probably what dumpkeys(1) and showkey(1) use. Echoing
> different values here should allow for remapping speakup's review
> commands besides remapping the keyboard as a whole.

AFAIK the Speakup keymap is just for remapping keys to Speakup
functions.  It's a binary format, not related to dumpkeys etc.  You need
a special program to compile a textual keymap into something that can be
loaded into /sys/accessibility/speakup/keymap.  I may have source for
that lying around here somewhere.  This is "here there be dragons"
territory.  I think the only specification of the format is in the
source code.

-- Chris

Re: Staging status of speakup

[Chris Brannon]

Okash Khawaja  writes:

> Finally there is an issue where text in output buffer sometimes gets
> garbled on SMP systems, but we can continue working on it after the
> driver is moved out of staging, if that's okay. Basically we need a
> reproducer of this issue.

What kind of reproducer do you need here?  It's straightforward to
reproduce in casual use, at least with a software synthesizer.  I don't
know whether it affects hardware synths.

-- Chris

Re: [patch 3/3] speakup: add unicode variant of /dev/softsynth

[Chris Brannon]

Samuel Thibault <samuel.thiba...@ens-lyon.org> writes:

> This adds /dev/softsynthu, along /dev/softsynth, which emits output in
> UTF-8 encoding, thus allowing to support 16bit characters.  Most of the
> code is shared, only the read function has to behave differently in
> latin1 and in unicode mode.  Since Linux only supports 16bit characters,
> we can just hardcode the UTF-8 encoding.
>
> Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>


Reviewed-by: Chris Brannon <ch...@the-brannons.com>

Looks good to me.

Re: [patch 3/3] speakup: add unicode variant of /dev/softsynth

[Chris Brannon]

Samuel Thibault  writes:

> This adds /dev/softsynthu, along /dev/softsynth, which emits output in
> UTF-8 encoding, thus allowing to support 16bit characters.  Most of the
> code is shared, only the read function has to behave differently in
> latin1 and in unicode mode.  Since Linux only supports 16bit characters,
> we can just hardcode the UTF-8 encoding.
>
> Signed-off-by: Samuel Thibault 


Reviewed-by: Chris Brannon 

Looks good to me.

Re: [patch 1/3] speakup: extend synth buffer to 16bit unicode characters

[Chris Brannon]

Samuel Thibault <samuel.thiba...@ens-lyon.org> writes:

> This extends the synth buffer slots to 16bit, so as to hold 16bit
> unicode characters.
>
> synth_buffer_getc and synth_buffer_peek now return 16bit characters.
> Speech synthesizers which do not support characters beyond latin1 can
> use the synth_buffer_skip_nonlatin1() helper to skip the non-latin1
> characters before getting or peeking. All synthesizers are made to use
> it for now.
>
> This makes synth_buffer_add take a 16bit character. For simplicity for
> now, synth_printf is left to using latin1 formats and strings.
> synth_putwc, synth_putwc_s, synth_putws and synth_putws_s helpers are
> however added to put 16bit characters and strings.
>
> Signed-off-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>


Reviewed-by: Chris Brannon <ch...@the-brannons.com>


Looks good to me.

Re: [patch 1/3] speakup: extend synth buffer to 16bit unicode characters

[Chris Brannon]

Samuel Thibault  writes:

> This extends the synth buffer slots to 16bit, so as to hold 16bit
> unicode characters.
>
> synth_buffer_getc and synth_buffer_peek now return 16bit characters.
> Speech synthesizers which do not support characters beyond latin1 can
> use the synth_buffer_skip_nonlatin1() helper to skip the non-latin1
> characters before getting or peeking. All synthesizers are made to use
> it for now.
>
> This makes synth_buffer_add take a 16bit character. For simplicity for
> now, synth_printf is left to using latin1 formats and strings.
> synth_putwc, synth_putwc_s, synth_putws and synth_putws_s helpers are
> however added to put 16bit characters and strings.
>
> Signed-off-by: Samuel Thibault 


Reviewed-by: Chris Brannon 


Looks good to me.

Re: [PATCH v2 3/3] staging: speakup: TODO Correct email

[Chris Brannon]

Walt Feasel <waltfea...@gmail.com> writes:

> Make email correction for k...@reisers.ca
>
> Signed-off-by: Walt Feasel <waltfea...@gmail.com>

Acked-by: Chris Brannon <ch...@the-brannons.com>

> ---
> v2 makes changes to correct for email format patch submission
>
>  drivers/staging/speakup/TODO | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/speakup/TODO b/drivers/staging/speakup/TODO
> index 3094799..993410c 100644
> --- a/drivers/staging/speakup/TODO
> +++ b/drivers/staging/speakup/TODO
> @@ -42,6 +42,6 @@ We prefer that you contact us on the mailing list; however, 
> if you do
>  not want to subscribe to a mailing list, send your email to all of the
>  following:
>  
> -w.d.hu...@gmail.com, ch...@the-brannons.com, k...@braille.uwo.ca and
> +w.d.hu...@gmail.com, ch...@the-brannons.com, k...@reisers.ca and
>  samuel.thiba...@ens-lyon.org.

Re: [PATCH v2 3/3] staging: speakup: TODO Correct email

[Chris Brannon]

Walt Feasel  writes:

> Make email correction for k...@reisers.ca
>
> Signed-off-by: Walt Feasel 

Acked-by: Chris Brannon 

> ---
> v2 makes changes to correct for email format patch submission
>
>  drivers/staging/speakup/TODO | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/speakup/TODO b/drivers/staging/speakup/TODO
> index 3094799..993410c 100644
> --- a/drivers/staging/speakup/TODO
> +++ b/drivers/staging/speakup/TODO
> @@ -42,6 +42,6 @@ We prefer that you contact us on the mailing list; however, 
> if you do
>  not want to subscribe to a mailing list, send your email to all of the
>  following:
>  
> -w.d.hu...@gmail.com, ch...@the-brannons.com, k...@braille.uwo.ca and
> +w.d.hu...@gmail.com, ch...@the-brannons.com, k...@reisers.ca and
>  samuel.thiba...@ens-lyon.org.

Re: [PATCH] [STYLE]staging:MAINTAINERS email revision speakup

[Chris Brannon]

Walt Feasel  writes:

> Modified email address per the TODO file in
> speakup's email listing, also verified email
> address from speakup's website

NAK.  The website needs updating, because it has Kirk's old addresses.
k...@reisers.ca is correct.

Re: [PATCH] [STYLE]staging:MAINTAINERS email revision speakup

[Chris Brannon]

Walt Feasel  writes:

> Modified email address per the TODO file in
> speakup's email listing, also verified email
> address from speakup's website

NAK.  The website needs updating, because it has Kirk's old addresses.
k...@reisers.ca is correct.

Re: [PATCH v4 4/7] staging: speakup: replace spk_strlwr() with strlcpytolower()

[Chris Brannon]

Markus Mayer <mma...@broadcom.com> writes:

> After introducing generic strltolower() and strtolower(), spk_strlwr()
> is no longer needed.
>
> Signed-off-by: Markus Mayer <mma...@broadcom.com>
> Acked-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>

Acked-by: Chris Brannon <ch...@the-brannons.com>

Haven't looked at much kernel code in a while, but this looks good.

-- Chris

Re: [PATCH v4 4/7] staging: speakup: replace spk_strlwr() with strlcpytolower()

[Chris Brannon]

Markus Mayer  writes:

> After introducing generic strltolower() and strtolower(), spk_strlwr()
> is no longer needed.
>
> Signed-off-by: Markus Mayer 
> Acked-by: Samuel Thibault 

Acked-by: Chris Brannon 

Haven't looked at much kernel code in a while, but this looks good.

-- Chris

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Dan Carpenter  writes:

> Good eye for spotting the memory corruption bug!
>
> This is a bug fix, so the fix should go in a separate patch and not
> merged with a code cleanup patch.  Ordinary users can trigger this so
> it's a security bug and separating it out is extra important.

Ok.  I just sent up a patch to the driverdev list.  I missed a few
of the Cc's that were on this thread, though.
Also, it will conflict with Raphael's cleanup.

> The checking in spk_set_num_var() is not sufficient as well.  If we use
> E_INC then we can hit an integer overflow bug:

Good catch.  In fact, we shouldn't be using input at all!  Instead, we
need to use the value of the voice parameter after it was changed.  That
will be a valid index into the two tables.  My patch does so.

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

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Dan Carpenter dan.carpen...@oracle.com writes:

 Good eye for spotting the memory corruption bug!

 This is a bug fix, so the fix should go in a separate patch and not
 merged with a code cleanup patch.  Ordinary users can trigger this so
 it's a security bug and separating it out is extra important.

Ok.  I just sent up a patch to the driverdev list.  I missed a few
of the Cc's that were on this thread, though.
Also, it will conflict with Raphael's cleanup.

 The checking in spk_set_num_var() is not sufficient as well.  If we use
 E_INC then we can hit an integer overflow bug:

Good catch.  In fact, we shouldn't be using input at all!  Instead, we
need to use the value of the voice parameter after it was changed.  That
will be a valid index into the two tables.  My patch does so.

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

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Raphael S Carvalho  writes:

> Wouldn't the following code (right before the statement: if
> (param->var_id == VOICE)) 
> check if value is out of range?
>
> value = simple_strtol(cp, NULL, 10);
> ret = spk_set_num_var(value, param, len);
> if (ret == -ERANGE) {
> var_data = param->data;
> pr_warn("value for %s out of range, expect %d to %d\n",
> param->name,
> var_data->u.n.low, var_data->u.n.high);
> }

That code prints an error message if the value is out of range.  Also,
since spk_set_num_var returns -ERANGE, we know that spk_set_num_var
didn't set anything.
But we use value again later in the function, if param->var_id ==
VOICE.  In that second use, we don't check to see if value is in range.
So if I set voice to something nonsensical, like 234567, an error
message will be printed, but the calls in the body of the if statement
will use the nonsense value, reading data from an invalid location.
It seems that there's another bug lurking in this code.
If we try to set voice to default, spk_set_num_var returns -ERESTART.
In this case, we shouldn't use value at all when setting the pitch and volume.
"value" is meaningless, regardless of what it contains.
We should use the value of the default voice as the index instead.  So
the following should be correct, and you can ignore what I suggested earlier.

if (param->var_id == VOICE && (ret == 0 || ret == -ERESTART)) {
if (ret == -ERESTART)
value = param->data.u.n.default_val;
spk_reset_default_value("pitch", synth->default_pitch,
value);
spk_reset_default_value("vol", synth->default_vol,
value);
}

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

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Raphael S Carvalho raphael.sc...@gmail.com writes:

 Wouldn't the following code (right before the statement: if
 (param-var_id == VOICE)) 
 check if value is out of range?

 value = simple_strtol(cp, NULL, 10);
 ret = spk_set_num_var(value, param, len);
 if (ret == -ERANGE) {
 var_data = param-data;
 pr_warn(value for %s out of range, expect %d to %d\n,
 param-name,
 var_data-u.n.low, var_data-u.n.high);
 }

That code prints an error message if the value is out of range.  Also,
since spk_set_num_var returns -ERANGE, we know that spk_set_num_var
didn't set anything.
But we use value again later in the function, if param-var_id ==
VOICE.  In that second use, we don't check to see if value is in range.
So if I set voice to something nonsensical, like 234567, an error
message will be printed, but the calls in the body of the if statement
will use the nonsense value, reading data from an invalid location.
It seems that there's another bug lurking in this code.
If we try to set voice to default, spk_set_num_var returns -ERESTART.
In this case, we shouldn't use value at all when setting the pitch and volume.
value is meaningless, regardless of what it contains.
We should use the value of the default voice as the index instead.  So
the following should be correct, and you can ignore what I suggested earlier.

if (param-var_id == VOICE  (ret == 0 || ret == -ERESTART)) {
if (ret == -ERESTART)
value = param-data.u.n.default_val;
spk_reset_default_value(pitch, synth-default_pitch,
value);
spk_reset_default_value(vol, synth-default_vol,
value);
}

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

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

"Raphael S.Carvalho"  writes:

> + /*
> + * If voice was just changed, we might need to reset our default
> + * pitch and volume.
> + */
> + if (param->var_id == VOICE) {
> + spk_reset_default_value("pitch", synth->default_pitch,
> + value);
> + spk_reset_default_value("vol", synth->default_vol,
> + value);

There's an "invalid read" bug here.  You didn't introduce it; it has
been there all along.  It's possible that value contains a value that is
out of range, in which case, the spk_reset_default_value calls could
fetch invalid data.  The value of ret should be sufficient for
determining whether value is in range, so I'd change the condition of
the if statement to this:

if (param->var_id == VOICE && ret != -ERANGE) {

Or possibly better:
if (param->var_id == VOICE && ret == 0) {

I'd say please resend with that fix, or if not, I can send a one-line
patch to be applied after yours.

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

Re: [PATCH 1/1] staging/speakup/kobjects.c: Code improvement.

[Chris Brannon]

Raphael S.Carvalho raphael.sc...@gmail.com writes:

 + /*
 + * If voice was just changed, we might need to reset our default
 + * pitch and volume.
 + */
 + if (param-var_id == VOICE) {
 + spk_reset_default_value(pitch, synth-default_pitch,
 + value);
 + spk_reset_default_value(vol, synth-default_vol,
 + value);

There's an invalid read bug here.  You didn't introduce it; it has
been there all along.  It's possible that value contains a value that is
out of range, in which case, the spk_reset_default_value calls could
fetch invalid data.  The value of ret should be sufficient for
determining whether value is in range, so I'd change the condition of
the if statement to this:

if (param-var_id == VOICE  ret != -ERANGE) {

Or possibly better:
if (param-var_id == VOICE  ret == 0) {

I'd say please resend with that fix, or if not, I can send a one-line
patch to be applied after yours.

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