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

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

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.