Re: thread safe usage of multiple phones

[Ladislav Michl]

Hi Peter,

(and sorry for such a late reply)

On Sun, May 14, 2017 at 09:31:55AM +0200, Peter Koch wrote:
> Dear Pawel
> 
> we are using multiple nokia phones now with our multi-threaded
> sms-daemon. Works well so far
> 
> I had to fix one problem and I did it the quick+dirty way.
> 
> Have a look at routine verify_max_message_len() in
> common/links/fbus-phonet.c, starting at line 64
> 
> static int verify_max_message_len(int len, char **message_buffer)
> {
>         static int max_message_len = 0;
> 
>         if (len > max_message_len) {
>                 dprintf("overrun: %d %d\n", len, max_message_len);
>                 *message_buffer = realloc(*message_buffer, len + 1);
>                 max_message_len = len + 1;
>         }
>         if (*message_buffer)
>                 return max_message_len;
>         else
>                 return 0;
> }
> 
> The static declaration of max_message_len causes problems if
> multiple thread are calling this routine. Even worse. When opening
> a phone max_message_len must be 0. Otherwise gn_lib_phone_open()
> will fail.
> 
> This makes the following code fail on the second invocation of
> gn_lib_phone_open(). max_message_len stays non-zero after
> the first call of verify_max_message_len() and therefor no memory
> will be allocated in the second call. *message_buffer will be NULL
> and gn_lib_phone_open() fails.
> 
> Of course this only happens when the phonet-driver is used, so my
> serial nokia 6310 works fine.
> 
> main(){
>   struct gn_statemachine *state;
>   gn_error err;
> 
>   err=gn_lib_phoneprofile_load_from_file(GNOKIIRC, NULL, &state);
>   printf("load=%d, %s\n", err, gn_error_print(err));
>   err=gn_lib_phone_open(state);
>   printf("open=%d, %s\n", err, gn_error_print(err));
>   err=gn_lib_phone_close(state);
>   printf("close=%d, %s\n", err, gn_error_print(err));
>   err=gn_lib_phone_open(state); // err will be 9 with nokia 6230i and
> dku2libusb
>   printf("open=%d, %s\n", err, gn_error_print(err));
> }
> 
> Here's my dirty fix for routine verify_max_message_len()
> 
> static int verify_max_message_len(int len, char **message_buffer)
> {
>         static int max_message_len = 0;
> 
>         if(len<PHONET_FRAME_MAX_LENGTH) len=PHONET_FRAME_MAX_LENGTH;
>         if(len>PHONET_FRAME_MAX_LENGTH || !*message_buffer){
>                 dprintf("reallocating message_buffer to %d bytes\n", len+1);
>                 *message_buffer = realloc(*message_buffer, len+1);
>         }
>         return *message_buffer ? len+1 : 0;
> 
>         if (len > max_message_len) {
>                 dprintf("overrun: %d %d\n", len, max_message_len);
>                 *message_buffer = realloc(*message_buffer, len + 1);
>                 max_message_len = len + 1;
>         }
>         if (*message_buffer)
>                 return max_message_len;
>         else
>                 return 0;
> }
> 
> Peter

Here's a completely untested patch. Care to give it a try?

Thank you.

diff --git a/common/links/fbus-phonet.c b/common/links/fbus-phonet.c
index 3300ceab..276de1d6 100644
--- a/common/links/fbus-phonet.c
+++ b/common/links/fbus-phonet.c
@@ -47,19 +47,18 @@ static gn_error phonet_send_message(unsigned int 
messagesize, unsigned char mess
 
 /*--------------------------------------------*/
 
-static int verify_max_message_len(int len, char **message_buffer)
+static int verify_max_message_len(int len, phonet_incoming_message *i)
 {
-       static int max_message_len = 0;
-
-       if (len > max_message_len || !*message_buffer) {
-               dprintf("overrun, reallocating: %d %d\n", len, max_message_len);
-               *message_buffer = realloc(*message_buffer, len + 1);
-               max_message_len = len + 1;
+       if (len > i->message_buffer_size || !i->message_buffer) {
+               dprintf("overrun, reallocating: %d %d\n", len, 
i->message_buffer_size);
+               i->message_buffer_size = len + 1;
+               i->message_buffer = realloc(i->message_buffer, 
i->message_buffer_size);
        }
-       if (*message_buffer)
-               return max_message_len;
-       else
-               return 0;
+       if (i->message_buffer)
+               return i->message_buffer_size;
+
+       i->message_buffer_size = 0;
+       return 0;
 }
 
 
@@ -171,7 +170,7 @@ static void phonet_rx_statemachine(unsigned char rx_byte, 
struct gn_statemachine
                i->message_length = i->message_length + rx_byte;
                i->state = FBUS_RX_GetMessage;
                i->buffer_count = 0;
-               if (!verify_max_message_len(i->message_length, 
&(i->message_buffer))) {
+               if (!verify_max_message_len(i->message_length, i)) {
                        dprintf("PHONET: Failed to allocate memory for larger 
buffer\n");
                        i->message_corrupted = 1;
                }
@@ -369,6 +368,7 @@ static void phonet_cleanup(struct gn_statemachine *state)
 {
        free(FBUSINST(state)->message_buffer);
        FBUSINST(state)->message_buffer = NULL;
+       FBUSINST(state)->message_buffer_size = 0;
 }
 
 /* Initialise variables and start the link */
@@ -388,7 +388,7 @@ gn_error phonet_initialise(struct gn_statemachine *state)
        if ((FBUSINST(state) = calloc(1, sizeof(phonet_incoming_message))) == 
NULL)
                return GN_ERR_MEMORYFULL;
 
-       if (!verify_max_message_len(PHONET_FRAME_MAX_LENGTH, 
&(FBUSINST(state)->message_buffer))) {
+       if (!verify_max_message_len(PHONET_FRAME_MAX_LENGTH, FBUSINST(state))) {
                dprintf("PHONET: Failed to initalize initial incoming buffer 
for %d bytes\n", PHONET_FRAME_MAX_LENGTH);
                return GN_ERR_MEMORYFULL;
        }
diff --git a/include/links/fbus-phonet.h b/include/links/fbus-phonet.h
index 89eaf0ba..88eb06ca 100644
--- a/include/links/fbus-phonet.h
+++ b/include/links/fbus-phonet.h
@@ -48,6 +48,7 @@ typedef struct {
        int message_type;
        int message_length;
        char *message_buffer;
+       int message_buffer_size;
        int message_corrupted;
 } phonet_incoming_message;