Re: thread safe usage of multiple phones

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;