[Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Thomas De Schampheleire]

Hi,

The original PSOS interfaces that take a name (like t_create,
sm_create etc), expect a character array with length 4:
unsigned long t_create(char name[4], unsigned long prio, unsigned long
sstack, unsigned long ustack, unsigned long flags, unsigned long
*tid);
unsigned long sm_create(char name[4], unsigned long count, unsigned
long flags, unsigned long *smid);

while the corresponding PSOS skin in Xenomai expects a null-terminated
character array (a real string):
u_long t_create(const char *name, u_long prio, u_long sstack, u_long
ustack, u_long flags, u_long *tid_r);
u_long sm_create(const char *name, u_long icount, u_long flags, u_long *smid_r);


In certain situations, this difference gives rise to a buffer overflow
on the name variable.
For example:

        unsigned long smid, err;
        {
                char id[4] = {'S','E','M','0'};
                err = sm_create(id,0,SM_PRIOR,&smid);
        }
        {
                char id[4] = "SEM";
                id[3] = '1';
                err = sm_create(id,0,SM_PRIOR,&smid);
        }
        {
                char id[4] = "SEM2";
                err = sm_create(id,0,SM_PRIOR,&smid);
        }
        {
                char id[4] = "MySemaphore";
                err = sm_create(id,0,SM_PRIOR,&smid);
        }

The first two examples are perfectly valid code. The third one (SEM2)
is dubious because the end-of-string character will overflow the
array, although the compiler does not complain. The fourth example
(MySemaphore) obviously is an array-overflow and is indeed warned upon
by the compiler.

On target, this creates the following semaphores (taken from the registry):

# ls /proc/xenomai/registry/psos/semaphores/SEM*
/proc/xenomai/registry/psos/semaphores/My*
/proc/xenomai/registry/psos/semaphores/MySeAB4abcaaaaaaaaaaaaaaaaaaaaa
/proc/xenomai/registry/psos/semaphores/SEM0????p????_S22753
/proc/xenomai/registry/psos/semaphores/SEM1p????_S22753
/proc/xenomai/registry/psos/semaphores/SEM2?_S22753

As you can see, in all cases there was an array overflow (the question
marks correspond to non-ASCII characters), caused by the missing
null-termination (in itself caused by a mismatch between the original
PSOS interface and the Xenomai PSOS skin implementation of it).

If you do not explicitly create a character array of length 4, e.g.
(char id[] = "SEM1") then the Xenomai code obviously works fine: it
receives a null-terminated string, as it expects.


To fix this problem, the PSOS skin should treat incoming names as
non-null-terminated character arrays of length 4, and explicitly add
null-termination before passing it to the nucleus.

What is your view on this?

Thanks,
Thomas

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Philippe Gerum]

On Fri, 2011-09-16 at 14:59 +0200, Thomas De Schampheleire wrote:
> Hi,
> 
> The original PSOS interfaces that take a name (like t_create,
> sm_create etc), expect a character array with length 4:
> unsigned long t_create(char name[4], unsigned long prio, unsigned long
> sstack, unsigned long ustack, unsigned long flags, unsigned long
> *tid);
> unsigned long sm_create(char name[4], unsigned long count, unsigned
> long flags, unsigned long *smid);
> 
> while the corresponding PSOS skin in Xenomai expects a null-terminated
> character array (a real string):
> u_long t_create(const char *name, u_long prio, u_long sstack, u_long
> ustack, u_long flags, u_long *tid_r);
> u_long sm_create(const char *name, u_long icount, u_long flags, u_long *smid_r);
> 
> 
> In certain situations, this difference gives rise to a buffer overflow
> on the name variable.
> For example:
> 
>         unsigned long smid, err;
>         {
>                 char id[4] = {'S','E','M','0'};
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
>         {
>                 char id[4] = "SEM";
>                 id[3] = '1';
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
>         {
>                 char id[4] = "SEM2";
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
>         {
>                 char id[4] = "MySemaphore";
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
> 
> The first two examples are perfectly valid code. The third one (SEM2)
> is dubious because the end-of-string character will overflow the
> array, although the compiler does not complain. The fourth example
> (MySemaphore) obviously is an array-overflow and is indeed warned upon
> by the compiler.
> 
> On target, this creates the following semaphores (taken from the registry):
> 
> # ls /proc/xenomai/registry/psos/semaphores/SEM*
> /proc/xenomai/registry/psos/semaphores/My*
> /proc/xenomai/registry/psos/semaphores/MySeAB4abcaaaaaaaaaaaaaaaaaaaaa
> /proc/xenomai/registry/psos/semaphores/SEM0????p????_S22753
> /proc/xenomai/registry/psos/semaphores/SEM1p????_S22753
> /proc/xenomai/registry/psos/semaphores/SEM2?_S22753
> 
> As you can see, in all cases there was an array overflow (the question
> marks correspond to non-ASCII characters), caused by the missing
> null-termination (in itself caused by a mismatch between the original
> PSOS interface and the Xenomai PSOS skin implementation of it).
> 
> If you do not explicitly create a character array of length 4, e.g.
> (char id[] = "SEM1") then the Xenomai code obviously works fine: it
> receives a null-terminated string, as it expects.
> 
> 
> To fix this problem, the PSOS skin should treat incoming names as
> non-null-terminated character arrays of length 4, and explicitly add
> null-termination before passing it to the nucleus.
> 
> What is your view on this?

Actually, we used to follow strictly the pSOS convention for this until
2.4.x, at which point we moved to name strings precisely because
non-null terminated char[4] arrays would break the registry, the way you
described. This is one of the rare situations where mimicking a useless
limitation of the original API may be challenged by usability concerns
in the new environment, and usability won in that case. The problem
mostly comes from the fact that char[4] is automatically convertible to
const char *, so we have no warning/error leading us to check the
potentially problematic call sites.

The concern about moving back to char[4] arrays - null-terminated if
shorter - is for people who currently assign strings longer than 4
characters to name their objects. What could be done, is providing a
build switch to select the accepted input, like
--disable-psos-long-names to turn on char[4] interpretation.

> 
> Thanks,
> Thomas
> 
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help

-- 
Philippe.

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[dietmar.schindler]

> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of Philippe Gerum
> Sent: Sunday, September 18, 2011 5:37 PM
> ...
> Actually, we used to follow strictly the pSOS convention for this until
> 2.4.x, at which point we moved to name strings precisely because
> non-null terminated char[4] arrays would break the registry, the way you
> described. This is one of the rare situations where mimicking a useless
> limitation of the original API may be challenged by usability concerns
> in the new environment, and usability won in that case. The problem
> mostly comes from the fact that char[4] is automatically convertible to
> const char *, so we have no warning/error leading us to check the
> potentially problematic call sites.
>
> The concern about moving back to char[4] arrays - null-terminated if
> shorter - is for people who currently assign strings longer than 4
> characters to name their objects. What could be done, is providing a
> build switch to select the accepted input, like
> --disable-psos-long-names to turn on char[4] interpretation.

While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
--
Regards,
Dietmar
________________________________________ manroland AG Vorsitzender des Aufsichtsrates: Hanno C. Fiedler Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592 USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Ronny Meeus]

On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
>> On Behalf Of Philippe Gerum
>> Sent: Sunday, September 18, 2011 5:37 PM
>> ...
>> Actually, we used to follow strictly the pSOS convention for this until
>> 2.4.x, at which point we moved to name strings precisely because
>> non-null terminated char[4] arrays would break the registry, the way you
>> described. This is one of the rare situations where mimicking a useless
>> limitation of the original API may be challenged by usability concerns
>> in the new environment, and usability won in that case. The problem
>> mostly comes from the fact that char[4] is automatically convertible to
>> const char *, so we have no warning/error leading us to check the
>> potentially problematic call sites.
>>
>> The concern about moving back to char[4] arrays - null-terminated if
>> shorter - is for people who currently assign strings longer than 4
>> characters to name their objects. What could be done, is providing a
>> build switch to select the accepted input, like
>> --disable-psos-long-names to turn on char[4] interpretation.
>
> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
> --
> Regards,
> Dietmar
> ________________________________________ manroland AG Vorsitzender des Aufsichtsrates: Hanno C. Fiedler Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592 USt-Ident-Nr. DE 250200933
>
>
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help
>

Another option would be to introduce a run-time switch.
The default behavior could be that long names are supported and if the
"enable_short_names()" function is called, all names will the cut at 4
characters.
The advantage of this dynamic switch is that different applications
using the same library can use the mode they prefer.

Regards,
Ronny

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Thomas De Schampheleire]

Hi,

On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
>>> On Behalf Of Philippe Gerum
>>> Sent: Sunday, September 18, 2011 5:37 PM
>>> ...
>>> Actually, we used to follow strictly the pSOS convention for this until
>>> 2.4.x, at which point we moved to name strings precisely because
>>> non-null terminated char[4] arrays would break the registry, the way you
>>> described. This is one of the rare situations where mimicking a useless
>>> limitation of the original API may be challenged by usability concerns
>>> in the new environment, and usability won in that case. The problem
>>> mostly comes from the fact that char[4] is automatically convertible to
>>> const char *, so we have no warning/error leading us to check the
>>> potentially problematic call sites.
>>>
>>> The concern about moving back to char[4] arrays - null-terminated if
>>> shorter - is for people who currently assign strings longer than 4
>>> characters to name their objects. What could be done, is providing a
>>> build switch to select the accepted input, like
>>> --disable-psos-long-names to turn on char[4] interpretation.
>>
>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
>> --
[..]
>>
>
> Another option would be to introduce a run-time switch.
> The default behavior could be that long names are supported and if the
> "enable_short_names()" function is called, all names will the cut at 4
> characters.
> The advantage of this dynamic switch is that different applications
> using the same library can use the mode they prefer.

I would also be in favor of a runtime setting, rather than a
compile-time one. One cannot assume that all PSOS applications on a
system follow the same rules, or that there cannot be a mix of
'legacy' PSOS applications and 'xenomai-style' ones. According to me,
a library should support all these uses.

Best regards,
Thomas

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Gilles Chanteperdrix]

On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
> Hi,
> 
> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
>>>> On Behalf Of Philippe Gerum
>>>> Sent: Sunday, September 18, 2011 5:37 PM
>>>> ...
>>>> Actually, we used to follow strictly the pSOS convention for this until
>>>> 2.4.x, at which point we moved to name strings precisely because
>>>> non-null terminated char[4] arrays would break the registry, the way you
>>>> described. This is one of the rare situations where mimicking a useless
>>>> limitation of the original API may be challenged by usability concerns
>>>> in the new environment, and usability won in that case. The problem
>>>> mostly comes from the fact that char[4] is automatically convertible to
>>>> const char *, so we have no warning/error leading us to check the
>>>> potentially problematic call sites.
>>>>
>>>> The concern about moving back to char[4] arrays - null-terminated if
>>>> shorter - is for people who currently assign strings longer than 4
>>>> characters to name their objects. What could be done, is providing a
>>>> build switch to select the accepted input, like
>>>> --disable-psos-long-names to turn on char[4] interpretation.
>>>
>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
>>> --
> [..]
>>>
>>
>> Another option would be to introduce a run-time switch.
>> The default behavior could be that long names are supported and if the
>> "enable_short_names()" function is called, all names will the cut at 4
>> characters.
>> The advantage of this dynamic switch is that different applications
>> using the same library can use the mode they prefer.
> 
> I would also be in favor of a runtime setting, rather than a
> compile-time one. One cannot assume that all PSOS applications on a
> system follow the same rules, or that there cannot be a mix of
> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
> a library should support all these uses.

Hi,

here is a patch which truncates identifiers to four characters and 
terminates them by a '\0' character, in order to avoid the issues at 
the origin of this thread. The patch also adds a variable 
"psos_long_names", 0 by default, which may be set to a non-zero value 
to enable the old behaviour (assume the identifier strings may be 
longer than 4 characters and are already null terminated).

Could someone with the issue test it?

Thanks in advance.
Regards.

diff --git a/include/psos+/psos.h b/include/psos+/psos.h
index 32281bc..4a3921a 100644
--- a/include/psos+/psos.h
+++ b/include/psos+/psos.h
@@ -197,6 +197,8 @@ u_long t_restart(u_long tid,
 
 #include <psos+/syscall.h>
 
+extern unsigned psos_long_names;
+
 #endif /* __KERNEL__ || __XENO_SIM__ */
 
 /*
diff --git a/src/skins/psos+/init.c b/src/skins/psos+/init.c
index 978a4f1..e53967b 100644
--- a/src/skins/psos+/init.c
+++ b/src/skins/psos+/init.c
@@ -26,6 +26,8 @@ int __psos_muxid = -1;
 
 xnsysinfo_t __psos_sysinfo;
 
+unsigned psos_long_names;
+
 static __attribute__ ((constructor))
 void __init_xeno_interface(void)
 {
@@ -68,3 +70,14 @@ void k_fatal(u_long err_code, u_long flags)
 {
 	exit(1);
 }
+
+const char *__psos_maybe_short_name(char shrt[5], const char *lng)
+{
+	if (psos_long_names)
+		return lng;
+
+	strncpy(shrt, lng, sizeof(shrt) - 1);
+	shrt[4] = '\0';
+
+	return (const char *)shrt;
+}
diff --git a/src/skins/psos+/queue.c b/src/skins/psos+/queue.c
index c54f966..c8c1933 100644
--- a/src/skins/psos+/queue.c
+++ b/src/skins/psos+/queue.c
@@ -17,11 +17,14 @@
  */
 
 #include <psos+/psos.h>
+#include <psos+/long_names.h>
 
 extern int __psos_muxid;
 
 u_long q_create(const char *name, u_long maxnum, u_long flags, u_long *qid_r)
 {
+	char short_name[5];
+	name = __psos_maybe_short_name(short_name, name);
 	return XENOMAI_SKINCALL4(__psos_muxid, __psos_q_create,
 				 name, maxnum, flags, qid_r);
 }
diff --git a/src/skins/psos+/rn.c b/src/skins/psos+/rn.c
index f254e37..2219f36 100644
--- a/src/skins/psos+/rn.c
+++ b/src/skins/psos+/rn.c
@@ -24,6 +24,7 @@
 #include <stdio.h>
 #include <nucleus/heap.h>
 #include <psos+/psos.h>
+#include <psos+/long_names.h>
 
 extern int __psos_muxid;
 
@@ -59,6 +60,7 @@ u_long rn_create(const char name[4],
 		 u_long usize, u_long flags, u_long *rnid, u_long *allocsz)
 {
 	struct rninfo rninfo;
+	char short_name[5];
 	struct {
 		u_long rnsize;
 		u_long usize;
@@ -66,6 +68,8 @@ u_long rn_create(const char name[4],
 	} sizeopt;
 	u_long err;
 
+	name = __psos_maybe_short_name(short_name, name);
+
 	if (rnaddr)
 		fprintf(stderr,
 			"rn_create() - rnaddr parameter ignored from user-space context\n");
diff --git a/src/skins/psos+/sem.c b/src/skins/psos+/sem.c
index 5020274..2bea943 100644
--- a/src/skins/psos+/sem.c
+++ b/src/skins/psos+/sem.c
@@ -17,11 +17,14 @@
  */
 
 #include <psos+/psos.h>
+#include <psos+/long_names.h>
 
 extern int __psos_muxid;
 
 u_long sm_create(const char name[4], u_long icount, u_long flags, u_long *smid_r)
 {
+	char short_name[5];
+	name = __psos_maybe_short_name(short_name, name);
 	return XENOMAI_SKINCALL4(__psos_muxid, __psos_sm_create,
 				 name, icount, flags, smid_r);
 }
diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c
index a43f7fb..ab04ed8 100644
--- a/src/skins/psos+/task.c
+++ b/src/skins/psos+/task.c
@@ -26,6 +26,7 @@
 #include <memory.h>
 #include <string.h>
 #include <psos+/psos.h>
+#include <psos+/long_names.h>
 #include <asm-generic/bits/sigshadow.h>
 #include <asm-generic/bits/current.h>
 #include <asm-generic/stack.h>
@@ -128,6 +129,9 @@ u_long t_create(const char *name,
 	int policy;
 	long err;
 
+	char short_name[5];
+	name = __psos_maybe_short_name(short_name, name);
+
 	/* Migrate this thread to the Linux domain since we are about
 	   to issue a series of regular kernel syscalls in order to
 	   create the new Linux thread, which in turn will be mapped


-- 
                                                                Gilles.

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Thomas De Schampheleire]

Hi,

On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
<gilles.chanteperdrix@xenomai.org> wrote:
> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
>> Hi,
>>
>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
>>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
>>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hidrg]
>>>>> On Behalf Of Philippe Gerum
>>>>> Sent: Sunday, September 18, 2011 5:37 PM
>>>>> ...
>>>>> Actually, we used to follow strictly the pSOS convention for this until
>>>>> 2.4.x, at which point we moved to name strings precisely because
>>>>> non-null terminated char[4] arrays would break the registry, the way you
>>>>> described. This is one of the rare situations where mimicking a useless
>>>>> limitation of the original API may be challenged by usability concerns
>>>>> in the new environment, and usability won in that case. The problem
>>>>> mostly comes from the fact that char[4] is automatically convertible to
>>>>> const char *, so we have no warning/error leading us to check the
>>>>> potentially problematic call sites.
>>>>>
>>>>> The concern about moving back to char[4] arrays - null-terminated if
>>>>> shorter - is for people who currently assign strings longer than 4
>>>>> characters to name their objects. What could be done, is providing a
>>>>> build switch to select the accepted input, like
>>>>> --disable-psos-long-names to turn on char[4] interpretation.
>>>>
>>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
>>>> --
>> [..]
>>>>
>>>
>>> Another option would be to introduce a run-time switch.
>>> The default behavior could be that long names are supported and if the
>>> "enable_short_names()" function is called, all names will the cut at 4
>>> characters.
>>> The advantage of this dynamic switch is that different applications
>>> using the same library can use the mode they prefer.
>>
>> I would also be in favor of a runtime setting, rather than a
>> compile-time one. One cannot assume that all PSOS applications on a
>> system follow the same rules, or that there cannot be a mix of
>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
>> a library should support all these uses.
>
> Hi,
>
> here is a patch which truncates identifiers to four characters and
> terminates them by a '\0' character, in order to avoid the issues at
> the origin of this thread. The patch also adds a variable
> "psos_long_names", 0 by default, which may be set to a non-zero value
> to enable the old behaviour (assume the identifier strings may be
> longer than 4 characters and are already null terminated).
>
> Could someone with the issue test it?

It seems this slipped through the cracks, my apologies. We already
implemented this ourselves similarly, we didn't actually test your
patch so far.

Now that we're trying out xenomai-forge, I ported this patch and tested it.
There are a few problems with it (also relevant to cobalt xenomai)

* a bug in __psos_maybe_short_name so that only 3 characters are
retained (see below)
* the call to __psos_maybe_short_name also needs to be done in the
_ident functions
* although we don't use it, I think the pt_create and pt_ident
functions also need to be adapted

Moreover, I wonder why you have made psos_long_names an exported
global variable. Sharing variables between two different pieces of
software is not very clean. I think it would be nicer with a get/set
function.


> diff --git a/include/psos+/psos.h b/include/psos+/psos.h
> index 32281bc..4a3921a 100644
> --- a/include/psos+/psos.h
> +++ b/include/psos+/psos.h
> @@ -197,6 +197,8 @@ u_long t_restart(u_long tid,
>
>  #include <psos+/syscall.h>
>
> +extern unsigned psos_long_names;
> +
>  #endif /* __KERNEL__ || __XENO_SIM__ */
>
>  /*
> diff --git a/src/skins/psos+/init.c b/src/skins/psos+/init.c
> index 978a4f1..e53967b 100644
> --- a/src/skins/psos+/init.c
> +++ b/src/skins/psos+/init.c
> @@ -26,6 +26,8 @@ int __psos_muxid = -1;
>
>  xnsysinfo_t __psos_sysinfo;
>
> +unsigned psos_long_names;
> +
>  static __attribute__ ((constructor))
>  void __init_xeno_interface(void)
>  {
> @@ -68,3 +70,14 @@ void k_fatal(u_long err_code, u_long flags)
>  {
>        exit(1);
>  }
> +
> +const char *__psos_maybe_short_name(char shrt[5], const char *lng)
> +{
> +       if (psos_long_names)
> +               return lng;
> +
> +       strncpy(shrt, lng, sizeof(shrt) - 1);

Because shrt is passed as parameter, it is passed as pointer and not
as array. This means there is no size information, and thus
sizeof(shrt) equals the size of a pointer (4 on our 32-bit platform).
As a result, only ABC of a name ABCD are copied.

You'll need to explicitly do:

strncpy(shrt, lng, 5 - 1);

> +       shrt[4] = '\0';
> +
> +       return (const char *)shrt;
> +}
> diff --git a/src/skins/psos+/queue.c b/src/skins/psos+/queue.c
> index c54f966..c8c1933 100644
> --- a/src/skins/psos+/queue.c
> +++ b/src/skins/psos+/queue.c
> @@ -17,11 +17,14 @@
>  */
>
>  #include <psos+/psos.h>
> +#include <psos+/long_names.h>
>
>  extern int __psos_muxid;
>
>  u_long q_create(const char *name, u_long maxnum, u_long flags, u_long *qid_r)
>  {
> +       char short_name[5];
> +       name = __psos_maybe_short_name(short_name, name);
>        return XENOMAI_SKINCALL4(__psos_muxid, __psos_q_create,
>                                 name, maxnum, flags, qid_r);
>  }
> diff --git a/src/skins/psos+/rn.c b/src/skins/psos+/rn.c
> index f254e37..2219f36 100644
> --- a/src/skins/psos+/rn.c
> +++ b/src/skins/psos+/rn.c
> @@ -24,6 +24,7 @@
>  #include <stdio.h>
>  #include <nucleus/heap.h>
>  #include <psos+/psos.h>
> +#include <psos+/long_names.h>
>
>  extern int __psos_muxid;
>
> @@ -59,6 +60,7 @@ u_long rn_create(const char name[4],
>                 u_long usize, u_long flags, u_long *rnid, u_long *allocsz)
>  {
>        struct rninfo rninfo;
> +       char short_name[5];
>        struct {
>                u_long rnsize;
>                u_long usize;
> @@ -66,6 +68,8 @@ u_long rn_create(const char name[4],
>        } sizeopt;
>        u_long err;
>
> +       name = __psos_maybe_short_name(short_name, name);
> +
>        if (rnaddr)
>                fprintf(stderr,
>                        "rn_create() - rnaddr parameter ignored from user-space context\n");
> diff --git a/src/skins/psos+/sem.c b/src/skins/psos+/sem.c
> index 5020274..2bea943 100644
> --- a/src/skins/psos+/sem.c
> +++ b/src/skins/psos+/sem.c
> @@ -17,11 +17,14 @@
>  */
>
>  #include <psos+/psos.h>
> +#include <psos+/long_names.h>
>
>  extern int __psos_muxid;
>
>  u_long sm_create(const char name[4], u_long icount, u_long flags, u_long *smid_r)
>  {
> +       char short_name[5];
> +       name = __psos_maybe_short_name(short_name, name);
>        return XENOMAI_SKINCALL4(__psos_muxid, __psos_sm_create,
>                                 name, icount, flags, smid_r);
>  }
> diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c
> index a43f7fb..ab04ed8 100644
> --- a/src/skins/psos+/task.c
> +++ b/src/skins/psos+/task.c
> @@ -26,6 +26,7 @@
>  #include <memory.h>
>  #include <string.h>
>  #include <psos+/psos.h>
> +#include <psos+/long_names.h>
>  #include <asm-generic/bits/sigshadow.h>
>  #include <asm-generic/bits/current.h>
>  #include <asm-generic/stack.h>
> @@ -128,6 +129,9 @@ u_long t_create(const char *name,
>        int policy;
>        long err;
>
> +       char short_name[5];
> +       name = __psos_maybe_short_name(short_name, name);
> +
>        /* Migrate this thread to the Linux domain since we are about
>           to issue a series of regular kernel syscalls in order to
>           create the new Linux thread, which in turn will be mapped
>

I'll send the updated patch for xenomai-forge later.

Best regards,
Thomas

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Gilles Chanteperdrix]

On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote:
> Hi,
> 
> On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
> <gilles.chanteperdrix@xenomai.org> wrote:
>> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
>>> Hi,
>>>
>>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
>>>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
>>>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
>>>>>> On Behalf Of Philippe Gerum
>>>>>> Sent: Sunday, September 18, 2011 5:37 PM
>>>>>> ...
>>>>>> Actually, we used to follow strictly the pSOS convention for this until
>>>>>> 2.4.x, at which point we moved to name strings precisely because
>>>>>> non-null terminated char[4] arrays would break the registry, the way you
>>>>>> described. This is one of the rare situations where mimicking a useless
>>>>>> limitation of the original API may be challenged by usability concerns
>>>>>> in the new environment, and usability won in that case. The problem
>>>>>> mostly comes from the fact that char[4] is automatically convertible to
>>>>>> const char *, so we have no warning/error leading us to check the
>>>>>> potentially problematic call sites.
>>>>>>
>>>>>> The concern about moving back to char[4] arrays - null-terminated if
>>>>>> shorter - is for people who currently assign strings longer than 4
>>>>>> characters to name their objects. What could be done, is providing a
>>>>>> build switch to select the accepted input, like
>>>>>> --disable-psos-long-names to turn on char[4] interpretation.
>>>>>
>>>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
>>>>> --
>>> [..]
>>>>>
>>>>
>>>> Another option would be to introduce a run-time switch.
>>>> The default behavior could be that long names are supported and if the
>>>> "enable_short_names()" function is called, all names will the cut at 4
>>>> characters.
>>>> The advantage of this dynamic switch is that different applications
>>>> using the same library can use the mode they prefer.
>>>
>>> I would also be in favor of a runtime setting, rather than a
>>> compile-time one. One cannot assume that all PSOS applications on a
>>> system follow the same rules, or that there cannot be a mix of
>>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
>>> a library should support all these uses.
>>
>> Hi,
>>
>> here is a patch which truncates identifiers to four characters and
>> terminates them by a '\0' character, in order to avoid the issues at
>> the origin of this thread. The patch also adds a variable
>> "psos_long_names", 0 by default, which may be set to a non-zero value
>> to enable the old behaviour (assume the identifier strings may be
>> longer than 4 characters and are already null terminated).
>>
>> Could someone with the issue test it?
> 
> It seems this slipped through the cracks, my apologies. We already
> implemented this ourselves similarly, we didn't actually test your
> patch so far.
> 
> Now that we're trying out xenomai-forge, I ported this patch and tested it.
> There are a few problems with it (also relevant to cobalt xenomai)
> 
> * a bug in __psos_maybe_short_name so that only 3 characters are
> retained (see below)
> * the call to __psos_maybe_short_name also needs to be done in the
> _ident functions
> * although we don't use it, I think the pt_create and pt_ident
> functions also need to be adapted
> 
> Moreover, I wonder why you have made psos_long_names an exported
> global variable. Sharing variables between two different pieces of
> software is not very clean. I think it would be nicer with a get/set
> function.

Thanks for the review. The exported global variable is the simplest
possible and sufficient implementation of this feature. You risk having
a hard time convincing us otherwise.

-- 
					    Gilles.

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Philippe Gerum]

On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote:
> On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote:
> > Hi,
> > 
> > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
> > <gilles.chanteperdrix@xenomai.org> wrote:
> >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
> >>> Hi,
> >>>
> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
> >>>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
> >>>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
> >>>>>> On Behalf Of Philippe Gerum
> >>>>>> Sent: Sunday, September 18, 2011 5:37 PM
> >>>>>> ...
> >>>>>> Actually, we used to follow strictly the pSOS convention for this until
> >>>>>> 2.4.x, at which point we moved to name strings precisely because
> >>>>>> non-null terminated char[4] arrays would break the registry, the way you
> >>>>>> described. This is one of the rare situations where mimicking a useless
> >>>>>> limitation of the original API may be challenged by usability concerns
> >>>>>> in the new environment, and usability won in that case. The problem
> >>>>>> mostly comes from the fact that char[4] is automatically convertible to
> >>>>>> const char *, so we have no warning/error leading us to check the
> >>>>>> potentially problematic call sites.
> >>>>>>
> >>>>>> The concern about moving back to char[4] arrays - null-terminated if
> >>>>>> shorter - is for people who currently assign strings longer than 4
> >>>>>> characters to name their objects. What could be done, is providing a
> >>>>>> build switch to select the accepted input, like
> >>>>>> --disable-psos-long-names to turn on char[4] interpretation.
> >>>>>
> >>>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
> >>>>> --
> >>> [..]
> >>>>>
> >>>>
> >>>> Another option would be to introduce a run-time switch.
> >>>> The default behavior could be that long names are supported and if the
> >>>> "enable_short_names()" function is called, all names will the cut at 4
> >>>> characters.
> >>>> The advantage of this dynamic switch is that different applications
> >>>> using the same library can use the mode they prefer.
> >>>
> >>> I would also be in favor of a runtime setting, rather than a
> >>> compile-time one. One cannot assume that all PSOS applications on a
> >>> system follow the same rules, or that there cannot be a mix of
> >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
> >>> a library should support all these uses.
> >>
> >> Hi,
> >>
> >> here is a patch which truncates identifiers to four characters and
> >> terminates them by a '\0' character, in order to avoid the issues at
> >> the origin of this thread. The patch also adds a variable
> >> "psos_long_names", 0 by default, which may be set to a non-zero value
> >> to enable the old behaviour (assume the identifier strings may be
> >> longer than 4 characters and are already null terminated).
> >>
> >> Could someone with the issue test it?
> > 
> > It seems this slipped through the cracks, my apologies. We already
> > implemented this ourselves similarly, we didn't actually test your
> > patch so far.
> > 
> > Now that we're trying out xenomai-forge, I ported this patch and tested it.
> > There are a few problems with it (also relevant to cobalt xenomai)
> > 
> > * a bug in __psos_maybe_short_name so that only 3 characters are
> > retained (see below)
> > * the call to __psos_maybe_short_name also needs to be done in the
> > _ident functions
> > * although we don't use it, I think the pt_create and pt_ident
> > functions also need to be adapted
> > 
> > Moreover, I wonder why you have made psos_long_names an exported
> > global variable. Sharing variables between two different pieces of
> > software is not very clean. I think it would be nicer with a get/set
> > function.
> 
> Thanks for the review. The exported global variable is the simplest
> possible and sufficient implementation of this feature. You risk having
> a hard time convincing us otherwise.

Ack.

> 
> -- 
> 					    Gilles.
> 
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help

-- 
Philippe.

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Ronny Meeus]

On Wed, Oct 12, 2011 at 5:56 PM, Philippe Gerum <rpm@xenomai.org> wrote:
> On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote:
>> On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote:
>> > Hi,
>> >
>> > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
>> > <gilles.chanteperdrix@xenomai.org> wrote:
>> >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
>> >>> Hi,
>> >>>
>> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
>> >>>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hidm> wrote:
>> >>>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hidna.org]
>> >>>>>> On Behalf Of Philippe Gerum
>> >>>>>> Sent: Sunday, September 18, 2011 5:37 PM
>> >>>>>> ...
>> >>>>>> Actually, we used to follow strictly the pSOS convention for this until
>> >>>>>> 2.4.x, at which point we moved to name strings precisely because
>> >>>>>> non-null terminated char[4] arrays would break the registry, the way you
>> >>>>>> described. This is one of the rare situations where mimicking a useless
>> >>>>>> limitation of the original API may be challenged by usability concerns
>> >>>>>> in the new environment, and usability won in that case. The problem
>> >>>>>> mostly comes from the fact that char[4] is automatically convertible to
>> >>>>>> const char *, so we have no warning/error leading us to check the
>> >>>>>> potentially problematic call sites.
>> >>>>>>
>> >>>>>> The concern about moving back to char[4] arrays - null-terminated if
>> >>>>>> shorter - is for people who currently assign strings longer than 4
>> >>>>>> characters to name their objects. What could be done, is providing a
>> >>>>>> build switch to select the accepted input, like
>> >>>>>> --disable-psos-long-names to turn on char[4] interpretation.
>> >>>>>
>> >>>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
>> >>>>> --
>> >>> [..]
>> >>>>>
>> >>>>
>> >>>> Another option would be to introduce a run-time switch.
>> >>>> The default behavior could be that long names are supported and if the
>> >>>> "enable_short_names()" function is called, all names will the cut at 4
>> >>>> characters.
>> >>>> The advantage of this dynamic switch is that different applications
>> >>>> using the same library can use the mode they prefer.
>> >>>
>> >>> I would also be in favor of a runtime setting, rather than a
>> >>> compile-time one. One cannot assume that all PSOS applications on a
>> >>> system follow the same rules, or that there cannot be a mix of
>> >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
>> >>> a library should support all these uses.
>> >>
>> >> Hi,
>> >>
>> >> here is a patch which truncates identifiers to four characters and
>> >> terminates them by a '\0' character, in order to avoid the issues at
>> >> the origin of this thread. The patch also adds a variable
>> >> "psos_long_names", 0 by default, which may be set to a non-zero value
>> >> to enable the old behaviour (assume the identifier strings may be
>> >> longer than 4 characters and are already null terminated).
>> >>
>> >> Could someone with the issue test it?
>> >
>> > It seems this slipped through the cracks, my apologies. We already
>> > implemented this ourselves similarly, we didn't actually test your
>> > patch so far.
>> >
>> > Now that we're trying out xenomai-forge, I ported this patch and tested it.
>> > There are a few problems with it (also relevant to cobalt xenomai)
>> >
>> > * a bug in __psos_maybe_short_name so that only 3 characters are
>> > retained (see below)
>> > * the call to __psos_maybe_short_name also needs to be done in the
>> > _ident functions
>> > * although we don't use it, I think the pt_create and pt_ident
>> > functions also need to be adapted
>> >
>> > Moreover, I wonder why you have made psos_long_names an exported
>> > global variable. Sharing variables between two different pieces of
>> > software is not very clean. I think it would be nicer with a get/set
>> > function.
>>
>> Thanks for the review. The exported global variable is the simplest
>> possible and sufficient implementation of this feature. You risk having
>> a hard time convincing us otherwise.
>
> Ack.
>
>>
>> --
>>                                           Gilles.
>>
>> _______________________________________________
>> Xenomai-help mailing list
>> Xenomai-help@domain.hid
>> https://mail.gna.org/listinfo/xenomai-help
>
> --
> Philippe.
>
>
>
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help
>

Another simple solution would be to have a function available to set
this mode. In that way the internal implementation can be changed
without impact on the applications. I think it is always good to use a
setter since this is more future safe.

We also implemented the function tm_getm to get a timestamp in an
efficient way. This function was available in the xenomai-2.5.6 (as a
kind of extension on the psos interface) but in missing in the force
version.
I hope this code will also be in the patch that Thomas will send.

Ronny

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Philippe Gerum]

On Wed, 2011-10-12 at 19:24 +0200, Ronny Meeus wrote:
> On Wed, Oct 12, 2011 at 5:56 PM, Philippe Gerum <rpm@xenomai.org> wrote:
> > On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote:
> >> On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote:
> >> > Hi,
> >> >
> >> > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
> >> > <gilles.chanteperdrix@xenomai.org> wrote:
> >> >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
> >> >>> Hi,
> >> >>>
> >> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus <ronny.meeus@domain.hid> wrote:
> >> >>>> On Mon, Sep 19, 2011 at 9:25 AM,  <dietmar.schindler@domain.hid> wrote:
> >> >>>>>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
> >> >>>>>> On Behalf Of Philippe Gerum
> >> >>>>>> Sent: Sunday, September 18, 2011 5:37 PM
> >> >>>>>> ...
> >> >>>>>> Actually, we used to follow strictly the pSOS convention for this until
> >> >>>>>> 2.4.x, at which point we moved to name strings precisely because
> >> >>>>>> non-null terminated char[4] arrays would break the registry, the way you
> >> >>>>>> described. This is one of the rare situations where mimicking a useless
> >> >>>>>> limitation of the original API may be challenged by usability concerns
> >> >>>>>> in the new environment, and usability won in that case. The problem
> >> >>>>>> mostly comes from the fact that char[4] is automatically convertible to
> >> >>>>>> const char *, so we have no warning/error leading us to check the
> >> >>>>>> potentially problematic call sites.
> >> >>>>>>
> >> >>>>>> The concern about moving back to char[4] arrays - null-terminated if
> >> >>>>>> shorter - is for people who currently assign strings longer than 4
> >> >>>>>> characters to name their objects. What could be done, is providing a
> >> >>>>>> build switch to select the accepted input, like
> >> >>>>>> --disable-psos-long-names to turn on char[4] interpretation.
> >> >>>>>
> >> >>>>> While I would prefer the switch --enable-psos-long-names, this sounds okay to me, the more so as it is more than people who use the pSOS skin without obeying pSOS rules deserve.
> >> >>>>> --
> >> >>> [..]
> >> >>>>>
> >> >>>>
> >> >>>> Another option would be to introduce a run-time switch.
> >> >>>> The default behavior could be that long names are supported and if the
> >> >>>> "enable_short_names()" function is called, all names will the cut at 4
> >> >>>> characters.
> >> >>>> The advantage of this dynamic switch is that different applications
> >> >>>> using the same library can use the mode they prefer.
> >> >>>
> >> >>> I would also be in favor of a runtime setting, rather than a
> >> >>> compile-time one. One cannot assume that all PSOS applications on a
> >> >>> system follow the same rules, or that there cannot be a mix of
> >> >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
> >> >>> a library should support all these uses.
> >> >>
> >> >> Hi,
> >> >>
> >> >> here is a patch which truncates identifiers to four characters and
> >> >> terminates them by a '\0' character, in order to avoid the issues at
> >> >> the origin of this thread. The patch also adds a variable
> >> >> "psos_long_names", 0 by default, which may be set to a non-zero value
> >> >> to enable the old behaviour (assume the identifier strings may be
> >> >> longer than 4 characters and are already null terminated).
> >> >>
> >> >> Could someone with the issue test it?
> >> >
> >> > It seems this slipped through the cracks, my apologies. We already
> >> > implemented this ourselves similarly, we didn't actually test your
> >> > patch so far.
> >> >
> >> > Now that we're trying out xenomai-forge, I ported this patch and tested it.
> >> > There are a few problems with it (also relevant to cobalt xenomai)
> >> >
> >> > * a bug in __psos_maybe_short_name so that only 3 characters are
> >> > retained (see below)
> >> > * the call to __psos_maybe_short_name also needs to be done in the
> >> > _ident functions
> >> > * although we don't use it, I think the pt_create and pt_ident
> >> > functions also need to be adapted
> >> >
> >> > Moreover, I wonder why you have made psos_long_names an exported
> >> > global variable. Sharing variables between two different pieces of
> >> > software is not very clean. I think it would be nicer with a get/set
> >> > function.
> >>
> >> Thanks for the review. The exported global variable is the simplest
> >> possible and sufficient implementation of this feature. You risk having
> >> a hard time convincing us otherwise.
> >
> > Ack.
> >
> >>
> >> --
> >>                                           Gilles.
> >>
> >> _______________________________________________
> >> Xenomai-help mailing list
> >> Xenomai-help@domain.hid
> >> https://mail.gna.org/listinfo/xenomai-help
> >
> > --
> > Philippe.
> >
> >
> >
> > _______________________________________________
> > Xenomai-help mailing list
> > Xenomai-help@domain.hid
> > https://mail.gna.org/listinfo/xenomai-help
> >
> 
> Another simple solution would be to have a function available to set
> this mode. In that way the internal implementation can be changed
> without impact on the applications. I think it is always good to use a
> setter since this is more future safe.

That function would do nothing else than setting an internal variable
with a global scope, be it a function pointer. In that case, a setter
brings no value, but would clutter the public interface. The problem
with the cleanliness argument raised earlier is that it vastly depends
on where someone wants to put the dirt.

I don't think we should spent too much time on this anyway, I'd rather
solve the issue of handling duplicate pSOS object names. It is possible
to implement this in -forge, the last concern is about the complexity vs
usefulness ratio which I still find a bit high, so I want to try a
simpler implementation.

> 
> We also implemented the function tm_getm to get a timestamp in an
> efficient way. This function was available in the xenomai-2.5.6 (as a
> kind of extension on the psos interface) but in missing in the force
> version.

Ok.

> I hope this code will also be in the patch that Thomas will send.
> 
> Ronny

-- 
Philippe.

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[dietmar.schindler]

> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
> On Behalf Of Thomas De Schampheleire
> Sent: Friday, September 16, 2011 3:00 PM
> ...
> The original PSOS interfaces that take a name (like t_create,
> sm_create etc), expect a character array with length 4:
> ...
> while the corresponding PSOS skin in Xenomai expects a null-terminated
> character array (a real string):
> ...
>
> In certain situations, this difference gives rise to a buffer overflow
> on the name variable.
> For example:
>
>         unsigned long smid, err;
>         {
>                 char id[4] = {'S','E','M','0'};
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
>         {
>                 char id[4] = "SEM";
>                 id[3] = '1';
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
>         {
>                 char id[4] = "SEM2";
>                 err = sm_create(id,0,SM_PRIOR,&smid);
>         }
> ...
>
> ... The third one (SEM2)
> is dubious because the end-of-string character will overflow the
> array, ...

This is not true, according to ISO/IEC 9899:TC3 Programming languages - C, §6.7.8 Initialization:
...
14 An array of character type may be initialized by a character string literal, optionally enclosed in braces. Successive characters of the character string literal (including the terminating null character if there is room or if the array is of unknown size) initialize the elements of the array.
...
32 EXAMPLE 8 The declaration
      char s[] = "abc", t[3] = "abc";
defines ''plain'' char array objects s and t whose elements are initialized with character string literals.
This declaration is identical to
      char s[] = { 'a', 'b', 'c', '\0' },
           t[] = { 'a', 'b', 'c' };

> On target, this creates the following semaphores (taken from the
> registry):
>
> # ls /proc/xenomai/registry/psos/semaphores/SEM*
> ...
> /proc/xenomai/registry/psos/semaphores/SEM0????p????_S22753
> /proc/xenomai/registry/psos/semaphores/SEM1p????_S22753
> /proc/xenomai/registry/psos/semaphores/SEM2?_S22753
>
> As you can see, in all cases there was an array overflow (the question
> marks correspond to non-ASCII characters), caused by the missing
> null-termination (in itself caused by a mismatch between the original
> PSOS interface and the Xenomai PSOS skin implementation of it).

I would prefer to call this an array overrun (to distinguish the reading past the end of the array here from writing past the end), even though that is not a generally accepted distinction.

> If you do not explicitly create a character array of length 4, e.g.
> (char id[] = "SEM1") then the Xenomai code obviously works fine: it
> receives a null-terminated string, as it expects.
>
>
> To fix this problem, the PSOS skin should treat incoming names as
> non-null-terminated character arrays of length 4, and explicitly add
> null-termination before passing it to the nucleus.
>
> What is your view on this?

I agree.
--
Regards,
Dietmar
________________________________________ manroland AG Vorsitzender des Aufsichtsrates: Hanno C. Fiedler Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592 USt-Ident-Nr. DE 250200933

Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow

[Thomas De Schampheleire]

Hi Dietmar,

On Mon, Sep 19, 2011 at 8:59 AM,  <dietmar.schindler@domain.hid> wrote:
>> From: xenomai-help-bounces@domain.hid [mailto:xenomai-help-bounces@domain.hid]
>> On Behalf Of Thomas De Schampheleire
>> Sent: Friday, September 16, 2011 3:00 PM
>> ...
>> The original PSOS interfaces that take a name (like t_create,
>> sm_create etc), expect a character array with length 4:
>> ...
>> while the corresponding PSOS skin in Xenomai expects a null-terminated
>> character array (a real string):
>> ...
>>
>> In certain situations, this difference gives rise to a buffer overflow
>> on the name variable.
>> For example:
>>
>>         unsigned long smid, err;
>>         {
>>                 char id[4] = {'S','E','M','0'};
>>                 err = sm_create(id,0,SM_PRIOR,&smid);
>>         }
>>         {
>>                 char id[4] = "SEM";
>>                 id[3] = '1';
>>                 err = sm_create(id,0,SM_PRIOR,&smid);
>>         }
>>         {
>>                 char id[4] = "SEM2";
>>                 err = sm_create(id,0,SM_PRIOR,&smid);
>>         }
>> ...
>>
>> ... The third one (SEM2)
>> is dubious because the end-of-string character will overflow the
>> array, ...
>
> This is not true, according to ISO/IEC 9899:TC3 Programming languages - C, §6.7.8 Initialization:
> ...
> 14 An array of character type may be initialized by a character string literal, optionally enclosed in braces. Successive characters of the character string literal (including the terminating null character if there is room or if the array is of unknown size) initialize the elements of the array.
> ...
> 32 EXAMPLE 8 The declaration
>      char s[] = "abc", t[3] = "abc";
> defines ''plain'' char array objects s and t whose elements are initialized with character string literals.
> This declaration is identical to
>      char s[] = { 'a', 'b', 'c', '\0' },
>           t[] = { 'a', 'b', 'c' };

Thanks for clarifying this...

Best regards,
Thomas

>
>> On target, this creates the following semaphores (taken from the
>> registry):
>>
>> # ls /proc/xenomai/registry/psos/semaphores/SEM*
>> ...
>> /proc/xenomai/registry/psos/semaphores/SEM0????p????_S22753
>> /proc/xenomai/registry/psos/semaphores/SEM1p????_S22753
>> /proc/xenomai/registry/psos/semaphores/SEM2?_S22753
>>
>> As you can see, in all cases there was an array overflow (the question
>> marks correspond to non-ASCII characters), caused by the missing
>> null-termination (in itself caused by a mismatch between the original
>> PSOS interface and the Xenomai PSOS skin implementation of it).
>
> I would prefer to call this an array overrun (to distinguish the reading past the end of the array here from writing past the end), even though that is not a generally accepted distinction.
>
>> If you do not explicitly create a character array of length 4, e.g.
>> (char id[] = "SEM1") then the Xenomai code obviously works fine: it
>> receives a null-terminated string, as it expects.
>>
>>
>> To fix this problem, the PSOS skin should treat incoming names as
>> non-null-terminated character arrays of length 4, and explicitly add
>> null-termination before passing it to the nucleus.
>>
>> What is your view on this?
>
> I agree.
> --
> Regards,
> Dietmar
> ________________________________________ manroland AG Vorsitzender des Aufsichtsrates: Hanno C. Fiedler Vorstand: Gerd Finkbeiner (Vorsitzender), Dr. Ingo Koch, Dr. Markus Rall, Paul Steidle Sitz der Gesellschaft: Offenbach am Main, Registergericht: Amtsgericht Offenbach HRB-Nr. 42592 USt-Ident-Nr. DE 250200933
>
>
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help
>