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 3

This is not the most recent revision of the diff. The latest diff is revision 4. See what's changed.

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] 117 lines
[+20] [+] static void protocol_destroy_fn(void *obj)
118
/*! \brief Destructor function for sessions */
118
/*! \brief Destructor function for sessions */
119
static void session_destroy_fn(void *obj)
119
static void session_destroy_fn(void *obj)
120
{
120
{
121
	struct ast_websocket *session = obj;
121
	struct ast_websocket *session = obj;
122

    
   
122

   

    
   
123
	ast_websocket_close(session, 0);

    
   
124

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

    
   
129

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

    
   
188

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

    
   
193
	int res;

    
   
194

   

    
   
195
	if (session->close_sent) {

    
   
196
		return 0;

    
   
197
	}
191

    
   
198

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

    
   
201

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

    
   
204

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

    
   
206
	session->close_sent = 1;
199

    
   
207

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

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

    
   
210
	ao2_unlock(session);

    
   
211
	return res;
201
}
212
}
202

    
   
213

   
203

    
   
214

   
204
/*! \brief Write function for websocket traffic */
215
/*! \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)
216
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));
242
		put_unaligned_uint16(&frame[2], htons(actual_length));
232
	} else if (length == 127) {
243
	} else if (length == 127) {
233
		put_unaligned_uint64(&frame[2], htonl(actual_length));
244
		put_unaligned_uint64(&frame[2], htonl(actual_length));
234
	}
245
	}
235

    
   
246

   

    
   
247
	ao2_lock(session);

    
   
248
	if (session->closing) {

    
   
249
		ao2_unlock(session);

    
   
250
		return -1;

    
   
251
	}

    
   
252

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

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

    
   
257

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

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

    
   
263
	ao2_unlock(session);
244

    
   
264

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

    
   
267

   
248
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_enable)(struct ast_websocket *session, size_t bytes)
268
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);
501
			memcpy(session->payload, &buf[frame_size], *payload_len);
482
			*payload = session->payload;
502
			*payload = session->payload;
483
			frame_size += (*payload_len);
503
			frame_size += (*payload_len);
484
		}
504
		}
485

    
   
505

   
486
		if (!session->closing) {
506
		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 {
507
	} else {
494
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
508
		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
509
		/* 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. */
510
		 * fit that, I think. */
497
		ast_websocket_close(session, 1003);
511
		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.