Re: [Xenomai-help] Segmentation fault in rt_printf print thread

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

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Christoph Permes wrote:
>>>>> Hi, 
>>>>>
>>>>> I've created a patch that provides a workaround for the segmentation
>>>>> fault:
>>>>>
>>>>> --- a/src/rtdk/rt_print.c	2009-05-21 16:34:54.000000000 +0200
>>>>> +++ b/src/rtdk/rt_print.c	2009-09-04 10:09:19.000000000 +0200
>>>>> @@ -40,6 +40,7 @@
>>>>>  struct entry_head {
>>>>>  	FILE *dest;
>>>>>  	uint32_t seq_no;
>>>>> +	uint32_t len;
>>>>>  	char text[1];
>>>>>  } __attribute__((packed));
>>>>>  
>>>>> @@ -113,6 +114,7 @@
>>>>>  			/* Write out empty entry */
>>>>>  			head = buffer->ring + write_pos;
>>>>>  			head->seq_no = __seq_no;
>>>>> +			head->len = 0;
>>>>>  			head->text[0] = 0;
>>>>>  
>>>>>  			/* Forward to the ring buffer start */
>>>>> @@ -146,6 +148,7 @@
>>>>>  	/* If we were able to write some text, finalise the entry */
>>>>>  	if (len > 0) {
>>>>>  		head->seq_no = ++__seq_no;
>>>>> +		head->len = len;
>>>>>  		head->dest = stream;
>>>>>  
>>>>>  		/* Move forward by text and head length */
>>>>> @@ -158,6 +161,7 @@
>>>>>  		/* An empty entry marks the wrap-around */
>>>>>  		head = buffer->ring + write_pos;
>>>>>  		head->seq_no = __seq_no;
>>>>> +		head->len = 0;
>>>>>  		head->text[0] = 0;
>>>>>  
>>>>>  		write_pos = 0;
>>>>> @@ -373,7 +377,7 @@
>>>>>  
>>>>>  		read_pos = buffer->read_pos;
>>>>>  		head = buffer->ring + read_pos;
>>>>> -		len = strlen(head->text);
>>>>> +		len = head->len;
>>>>>  
>>>>>  		if (len) {
>>>>>  			/* Print out non-empty entry and proceed */
>>>>>
>>>>> With this patch I get no segfaults anymore, but of course it would be
>>>>> interesting why parts of the ring buffer are overwritten with null
>>>>> characters.
>>>>>
>>>> Frankly, I'm suspecting some issue in your application here - at least
>>>> as long as you do not have some broken-out test case for us...
>>> Would not it be possible to mprotect the rtdk buffer when it is not used
>>> in order to catch any write to it outside of rtdk functions?
>>>
>> Hmm, good idea, should work. Once set up, only rt_vsnprintf requires
>> write access to the ring.
> 
> Ok. Here it comes. Note however that calling rt_printf with this patch
> will cause the caller to switch to secondary mode, but that is OK for
> debugging. Also note that I only compile-tested the patch.

Except that the first mprotect should go before the read barrier. So, a
better version.

-- 
                                          Gilles


[-- Attachment #2: xeno-rtdk-mprotect.2.diff --]
[-- Type: text/x-patch, Size: 1456 bytes --]

diff --git a/src/rtdk/rt_print.c b/src/rtdk/rt_print.c
index 0615247..fd752d1 100644
--- a/src/rtdk/rt_print.c
+++ b/src/rtdk/rt_print.c
@@ -101,6 +101,7 @@ int rt_vfprintf(FILE *stream, const char *format, va_list args)
 	/* Take a snapshot of the ring buffer state */
 	write_pos = buffer->write_pos;
 	read_pos = buffer->read_pos;
+	mprotect(buffer->ring, buffer->size, PROT_READ | PROT_WRITE);
 	xnarch_read_memory_barrier();
 
 	/* Is our write limit the end of the ring buffer? */
@@ -167,6 +168,7 @@ int rt_vfprintf(FILE *stream, const char *format, va_list args)
 
 	/* All entry data must be written before we can update write_pos */
 	xnarch_write_memory_barrier();
+	mprotect(buffer->ring, buffer->size, PROT_READ);
 
 	buffer->write_pos = write_pos;
 
@@ -223,6 +225,7 @@ int rt_print_init(size_t buffer_size, const char *buffer_name)
 		size = default_buffer_size;
 	else if (size < RT_PRINT_LINE_BREAK)
 		return EINVAL;
+	size = (size + getpagesize() - 1) & ~(getpagesize() - 1);
 
 	if (buffer) {
 		/* Only set name if buffer size is unchanged or default */
@@ -237,12 +240,11 @@ int rt_print_init(size_t buffer_size, const char *buffer_name)
 	if (!buffer)
 		return ENOMEM;
 
-	buffer->ring = malloc(size);
+	buffer->ring = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE,  -1, 0);
 	if (!buffer->ring) {
 		free(buffer);
 		return ENOMEM;
 	}
-	memset(buffer->ring, 0, size);
 
 	buffer->read_pos  = 0;
 	buffer->write_pos = 0;