Re: [PATCH] Fix issue with AT parser in gatserver

Re: [PATCH] Fix issue with AT parser in gatserver

[Denis Kenzior]

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

Hi Frédéric,

On 07/07/2011 09:19 AM, Frédéric Dalleau wrote:
> This patch fix an issue with AT parser in gatserver.
> 
> If an empty command is issued after AT, then 2 answers are sent. The parser can
> also get in a bad state where next commands will not be handled. After that the
> parser will get back on his feets. This is caused by reentrency: 
> new_bytes() calls g_at_server_send_final(), which in turns calls server_resume
> which calls new_bytes() again synchronously.
> 

We should not try to set the read handler if it already set.  Have you
tried adding:

if (server->suspended == FALSE)
	return;

to server_resume()?

Regards,
-Denis

[PATCH] Fix issue with AT parser in gatserver

[=?unknown-8bit?q?Fr=C3=A9d=C3=A9ric?= Dalleau]

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

This patch fix an issue with AT parser in gatserver.

If an empty command is issued after AT, then 2 answers are sent. The parser can
also get in a bad state where next commands will not be handled. After that the
parser will get back on his feets. This is caused by reentrency: 
new_bytes() calls g_at_server_send_final(), which in turns calls server_resume
which calls new_bytes() again synchronously.

Sample dialog follows:

ofonod[30720]: Server: < AT+CMER=3,0,0,1\r\n
ofonod[30720]: Server: > \r\nOK\r\n
ofonod[30720]: Server: < AT+CFUN\r\n
ofonod[30720]: Server: > \r\nERROR\r\n
ofonod[30720]: Server: < AT\r\n
ofonod[30720]: Server: > \r\nOK\r\n
ofonod[30720]: Server: < \r\n
ofonod[30720]: Server: > \r\nOK\r\n\r\nOK\r\n
<2 answers sent>
ofonod[30720]: Server: < AT+CFUN\r\n
ofonod[30720]: Server: > \r\nERROR\r\n
ofonod[30720]: Server: < \r\n
ofonod[30720]: Server: > \r\nOK\r\n
ofonod[30720]: Server: < AT+CFUN\r\n
<missing ERROR reply to CFUN>
ofonod[30720]: Server: < \r\n
ofonod[30720]: Server: > \r\nOK\r\n


Frédéric Dalleau (1):
  gatchat: Fix reentrency issue with empty commands

 gatchat/gatserver.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

[PATCH] gatchat: Fix reentrency issue with empty commands

[=?unknown-8bit?q?Fr=C3=A9d=C3=A9ric?= Dalleau]

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

Emulator response to empty command is sent twice
---
 gatchat/gatserver.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index 3a996cb..a29e489 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -1008,6 +1008,9 @@ static void new_bytes(struct ring_buffer *rbuf, gpointer user_data)
 	unsigned char *buf = ring_buffer_read_ptr(rbuf, p->read_so_far);
 	enum ParserResult result;
 
+	if (p->in_read_handler)
+		return;
+
 	p->in_read_handler = TRUE;
 
 	while (p->io && (p->read_so_far < len)) {
-- 
1.7.1

Re: [PATCH] Fix issue with AT parser in gatserver

[Dalleau, Frederic]

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

Hi Denis,

2011/7/7 Denis Kenzior <denkenz@gmail.com>:
> Hi Frédéric,
>
> On 07/07/2011 09:19 AM, Frédéric Dalleau wrote:
>> This patch fix an issue with AT parser in gatserver.
>>
>> If an empty command is issued after AT, then 2 answers are sent. The parser can
>> also get in a bad state where next commands will not be handled. After that the
>> parser will get back on his feets. This is caused by reentrency:
>> new_bytes() calls g_at_server_send_final(), which in turns calls server_resume
>> which calls new_bytes() again synchronously.
>>
>
> We should not try to set the read handler if it already set.  Have you
> tried adding:
>
> if (server->suspended == FALSE)
>        return;
>

I like the idea of this one, but it still exhibit an issue in the case of A/:
Server_parse_line suspends and resumes in the
same function.

Regards,
Frédéric

Re: [PATCH] Fix issue with AT parser in gatserver

[Denis Kenzior]

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

Hi Frederic,

On 07/08/2011 09:44 AM, Dalleau, Frederic wrote:
> Hi Denis,
> 
> 2011/7/7 Denis Kenzior <denkenz@gmail.com>:
>> Hi Frédéric,
>>
>> On 07/07/2011 09:19 AM, Frédéric Dalleau wrote:
>>> This patch fix an issue with AT parser in gatserver.
>>>
>>> If an empty command is issued after AT, then 2 answers are sent. The parser can
>>> also get in a bad state where next commands will not be handled. After that the
>>> parser will get back on his feets. This is caused by reentrency:
>>> new_bytes() calls g_at_server_send_final(), which in turns calls server_resume
>>> which calls new_bytes() again synchronously.
>>>
>>
>> We should not try to set the read handler if it already set.  Have you
>> tried adding:
>>
>> if (server->suspended == FALSE)
>>        return;
>>
> 
> I like the idea of this one, but it still exhibit an issue in the case of A/:
> Server_parse_line suspends and resumes in the
> same function.
> 

I think I fixed this one as well in commit 329f2d7.  Let me know if you
encounter further problems.

Regards,
-Denis