Review Board 1.7.16


Websocket: Add locking around session access and modification

Review Request #3481 - Created April 25, 2014 and submitted

opticron
branches/11
ASTERISK-23605
Reviewers
asterisk-dev
Asterisk
This resolves a race condition where data could be written to a NULL FILE pointer causing a crash as a websocket connection was in the process of shutting down by adding locking to accesses and modifications of the websocket session struct.

 

Diff revision 4 (Latest)

1 2 3 4
1 2 3 4

  1. branches/11/res/res_http_websocket.c: Loading...
branches/11/res/res_http_websocket.c
Revision 413072 New Change
[20] 76 lines
[+20] [+] struct ast_websocket {
77
	size_t payload_len;               /*!< Length of the payload */
77
	size_t payload_len;               /*!< Length of the payload */
78
	char *payload;                    /*!< Pointer to the payload */
78
	char *payload;                    /*!< Pointer to the payload */
79
	size_t reconstruct;               /*!< Number of bytes before a reconstructed payload will be returned and a new one started */
79
	size_t reconstruct;               /*!< Number of bytes before a reconstructed payload will be returned and a new one started */
80
	unsigned int secure:1;            /*!< Bit to indicate that the transport is secure */
80
	unsigned int secure:1;            /*!< Bit to indicate that the transport is secure */
81
	unsigned int closing:1;           /*!< Bit to indicate that the session is in the process of being closed */
81
	unsigned int closing:1;           /*!< Bit to indicate that the session is in the process of being closed */

    
   
82
	unsigned int close_sent:1;        /*!< Bit to indicate that the session close opcode has been sent and no further data will be sent */
82
};
83
};
83

    
   
84

   
84
/*! \brief Structure definition for protocols */
85
/*! \brief Structure definition for protocols */
85
struct websocket_protocol {
86
struct websocket_protocol {
86
	char *name;                      /*!< Name of the protocol */
87
	char *name;                      /*!< Name of the protocol */
[+20] [20] 31 lines
[+20] [+] static void protocol_destroy_fn(void *obj)
118
/*! \brief Destructor function for sessions */
119
/*! \brief Destructor function for sessions */
119
static void session_destroy_fn(void *obj)
120
static void session_destroy_fn(void *obj)
120
{
121
{
121
	struct ast_websocket *session = obj;
122
	struct ast_websocket *session = obj;
122

    
   
123

   

    
   
124
	ast_websocket_close(session, 0);

    
   
125

   
123
	if (session->f) {
126
	if (session->f) {
124
		fclose(session->f);
127
		fclose(session->f);
125
		ast_verb(2, "WebSocket connection from '%s' closed\n", ast_sockaddr_stringify(&session->address));
128
		ast_verb(2, "WebSocket connection from '%s' closed\n", ast_sockaddr_stringify(&session->address));
126
	}
129
	}
127

    
   
130

   
[+20] [20] 58 lines
[+20] [+] int AST_OPTIONAL_API_NAME(ast_websocket_remove_protocol)(const char *name, ast_websocket_callback callback)
186

    
   
189

   
187
/*! \brief Close function for websocket session */
190
/*! \brief Close function for websocket session */
188
int AST_OPTIONAL_API_NAME(ast_websocket_close)(struct ast_websocket *session, uint16_t reason)
191
int AST_OPTIONAL_API_NAME(ast_websocket_close)(struct ast_websocket *session, uint16_t reason)
189
{
192
{
190
	char frame[4] = { 0, }; /* The header is 2 bytes and the reason code takes up another 2 bytes */
193
	char frame[4] = { 0, }; /* The header is 2 bytes and the reason code takes up another 2 bytes */

    
   
194
	int res;

    
   
195

   

    
   
196
	if (session->close_sent) {

    
   
197
		return 0;

    
   
198
	}
191

    
   
199

   
192
	frame[0] = AST_WEBSOCKET_OPCODE_CLOSE | 0x80;
200
	frame[0] = AST_WEBSOCKET_OPCODE_CLOSE | 0x80;
193
	frame[1] = 2; /* The reason code is always 2 bytes */
201
	frame[1] = 2; /* The reason code is always 2 bytes */
194

    
   
202

   
195
	/* If no reason has been specified assume 1000 which is normal closure */
203
	/* If no reason has been specified assume 1000 which is normal closure */
196
	put_unaligned_uint16(&frame[2], htons(reason ? reason : 1000));
204
	put_unaligned_uint16(&frame[2], htons(reason ? reason : 1000));
197

    
   
205

   
198
	session->closing = 1;
206
	session->closing = 1;

    
   
207
	session->close_sent = 1;
199

    
   
208

   
200
	return (fwrite(frame, 1, 4, session->f) == 4) ? 0 : -1;
209
	ao2_lock(session);

    
   
210
	res = (fwrite(frame, 1, 4, session->f) == 4) ? 0 : -1;

    
   
211
	ao2_unlock(session);

    
   
212
	return res;
201
}
213
}
202

    
   
214

   
203

    
   
215

   
204
/*! \brief Write function for websocket traffic */
216
/*! \brief Write function for websocket traffic */
205
int AST_OPTIONAL_API_NAME(ast_websocket_write)(struct ast_websocket *session, enum ast_websocket_opcode opcode, char *payload, uint64_t actual_length)
217
int AST_OPTIONAL_API_NAME(ast_websocket_write)(struct ast_websocket *session, enum ast_websocket_opcode opcode, char *payload, uint64_t actual_length)
[+20] [20] 25 lines
[+20] int AST_OPTIONAL_API_NAME(ast_websocket_close)(struct ast_websocket *session, uint16_t reason)
231
		put_unaligned_uint16(&frame[2], htons(actual_length));
243
		put_unaligned_uint16(&frame[2], htons(actual_length));
232
	} else if (length == 127) {
244
	} else if (length == 127) {
233
		put_unaligned_uint64(&frame[2], htonl(actual_length));
245
		put_unaligned_uint64(&frame[2], htonl(actual_length));
234
	}
246
	}
235

    
   
247

   

    
   
248
	ao2_lock(session);

    
   
249
	if (session->closing) {

    
   
250
		ao2_unlock(session);

    
   
251
		return -1;

    
   
252
	}

    
   
253

   
236
	if (fwrite(frame, 1, header_size, session->f) != header_size) {
254
	if (fwrite(frame, 1, header_size, session->f) != header_size) {

    
   
255
		ao2_unlock(session);
237
		return -1;
256
		return -1;
238
	}
257
	}
239

    
   
258

   
240
	if (fwrite(payload, 1, actual_length, session->f) != actual_length) {
259
	if (fwrite(payload, 1, actual_length, session->f) != actual_length) {

    
   
260
		ao2_unlock(session);
241
		return -1;
261
		return -1;
242
	}
262
	}
243
	fflush(session->f);
263
	fflush(session->f);

    
   
264
	ao2_unlock(session);
244

    
   
265

   
245
	return 0;
266
	return 0;
246
}
267
}
247

    
   
268

   
248
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_enable)(struct ast_websocket *session, size_t bytes)
269
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_enable)(struct ast_websocket *session, size_t bytes)
[+20] [20] 232 lines
[+20] [+] int AST_OPTIONAL_API_NAME(ast_websocket_read)(struct ast_websocket *session, char **payload, uint64_t *payload_len, enum ast_websocket_opcode *opcode, int *fragmented)
481
			memcpy(session->payload, &buf[frame_size], *payload_len);
502
			memcpy(session->payload, &buf[frame_size], *payload_len);
482
			*payload = session->payload;
503
			*payload = session->payload;
483
			frame_size += (*payload_len);
504
			frame_size += (*payload_len);
484
		}
505
		}
485

    
   
506

   
486
		if (!session->closing) {
507
		session->closing = 1;
487
			ast_websocket_close(session, 0);

   
488
		}

   
489

    
   

   
490
		fclose(session->f);

   
491
		session->f = NULL;

   
492
		ast_verb(2, "WebSocket connection from '%s' closed\n", ast_sockaddr_stringify(&session->address));

   
493
	} else {
508
	} else {
494
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
509
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
495
		/* We received an opcode that we don't understand, the RFC states that 1003 is for a type of data that can't be accepted... opcodes
510
		/* We received an opcode that we don't understand, the RFC states that 1003 is for a type of data that can't be accepted... opcodes
496
		 * fit that, I think. */
511
		 * fit that, I think. */
497
		ast_websocket_close(session, 1003);
512
		ast_websocket_close(session, 1003);
[+20] [20] 235 lines
  1. branches/11/res/res_http_websocket.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.