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 1

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 413007 New Change
[20] 185 lines
[+20] [+] int AST_OPTIONAL_API_NAME(ast_websocket_remove_protocol)(const char *name, ast_websocket_callback callback)
186

    
   
186

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

    
   
191
	int res;
191

    
   
192

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

    
   
195

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

    
   
198

   

    
   
199
	ao2_lock(session);
198
	session->closing = 1;
200
	session->closing = 1;
199

    
   
201

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

    
   
203
	ao2_unlock(session);

    
   
204
	return res;
201
}
205
}
202

    
   
206

   
203

    
   
207

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

    
   
239

   

    
   
240
	ao2_lock(session);
236
	if (fwrite(frame, 1, header_size, session->f) != header_size) {
241
	if (fwrite(frame, 1, header_size, session->f) != header_size) {

    
   
242
		ao2_unlock(session);
237
		return -1;
243
		return -1;
238
	}
244
	}
239

    
   
245

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

    
   
247
		ao2_unlock(session);
241
		return -1;
248
		return -1;
242
	}
249
	}
243
	fflush(session->f);
250
	fflush(session->f);

    
   
251
	ao2_unlock(session);
244

    
   
252

   
245
	return 0;
253
	return 0;
246
}
254
}
247

    
   
255

   
248
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_enable)(struct ast_websocket *session, size_t bytes)
256
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_enable)(struct ast_websocket *session, size_t bytes)
249
{
257
{

    
   
258
	ao2_lock(session);
250
	session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);
259
	session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);

    
   
260
	ao2_unlock(session);
251
}
261
}
252

    
   
262

   
253
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_disable)(struct ast_websocket *session)
263
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_disable)(struct ast_websocket *session)
254
{
264
{

    
   
265
	ao2_lock(session);
255
	session->reconstruct = 0;
266
	session->reconstruct = 0;

    
   
267
	ao2_unlock(session);
256
}
268
}
257

    
   
269

   
258
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
270
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
259
{
271
{
260
	ao2_ref(session, +1);
272
	ao2_ref(session, +1);
[+20] [20] 4 lines
[+20] [+] void AST_OPTIONAL_API_NAME(ast_websocket_unref)(struct ast_websocket *session)
265
	ao2_ref(session, -1);
277
	ao2_ref(session, -1);
266
}
278
}
267

    
   
279

   
268
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
280
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
269
{
281
{
270
	return session->closing ? -1 : session->fd;
282
	int res;

    
   
283
	ao2_lock(session);

    
   
284
	res = session->closing ? -1 : session->fd;

    
   
285
	ao2_unlock(session);

    
   
286
	return res;
271
}
287
}
272

    
   
288

   
273
struct ast_sockaddr * AST_OPTIONAL_API_NAME(ast_websocket_remote_address)(struct ast_websocket *session)
289
struct ast_sockaddr * AST_OPTIONAL_API_NAME(ast_websocket_remote_address)(struct ast_websocket *session)
274
{
290
{
275
	return &session->address;
291
	struct ast_sockaddr *res;

    
   
292
	ao2_lock(session);

    
   
293
	res = &session->address;

    
   
294
	ao2_unlock(session);

    
   
295
	return res;
276
}
296
}
277

    
   
297

   
278
int AST_OPTIONAL_API_NAME(ast_websocket_is_secure)(struct ast_websocket *session)
298
int AST_OPTIONAL_API_NAME(ast_websocket_is_secure)(struct ast_websocket *session)
279
{
299
{
280
	return session->secure;
300
	int res;

    
   
301
	ao2_lock(session);

    
   
302
	res = session->secure;

    
   
303
	ao2_unlock(session);

    
   
304
	return res;
281
}
305
}
282

    
   
306

   
283
int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
307
int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
284
{
308
{
285
	int flags;
309
	int flags;
286

    
   
310

   

    
   
311
	ao2_lock(session);
287
	if ((flags = fcntl(session->fd, F_GETFL)) == -1) {
312
	if ((flags = fcntl(session->fd, F_GETFL)) == -1) {

    
   
313
		ao2_unlock(session);
288
		return -1;
314
		return -1;
289
	}
315
	}
290

    
   
316

   
291
	flags |= O_NONBLOCK;
317
	flags |= O_NONBLOCK;
292

    
   
318

   
293
	if ((flags = fcntl(session->fd, F_SETFL, flags)) == -1) {
319
	if ((flags = fcntl(session->fd, F_SETFL, flags)) == -1) {

    
   
320
		ao2_unlock(session);
294
		return -1;
321
		return -1;
295
	}
322
	}

    
   
323
	ao2_unlock(session);
296

    
   
324

   
297
	return 0;
325
	return 0;
298
}
326
}
299

    
   
327

   
300
/* MAINTENANCE WARNING on ast_websocket_read()!
328
/* MAINTENANCE WARNING on ast_websocket_read()!
[+20] [20] 28 lines
[+20] int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
329
{
357
{
330
	int sanity;
358
	int sanity;
331
	size_t rlen;
359
	size_t rlen;
332
	int xlen = len;
360
	int xlen = len;
333
	char *rbuf = buf;
361
	char *rbuf = buf;

    
   
362
	ao2_lock(session);
334
	for (sanity = 10; sanity; sanity--) {
363
	for (sanity = 10; sanity; sanity--) {
335
		clearerr(session->f);
364
		clearerr(session->f);
336
		rlen = fread(rbuf, 1, xlen, session->f);
365
		rlen = fread(rbuf, 1, xlen, session->f);
337
		if (0 == rlen && ferror(session->f) && errno != EAGAIN) {
366
		if (0 == rlen && ferror(session->f) && errno != EAGAIN) {
338
			ast_log(LOG_ERROR, "Error reading from web socket: %s\n", strerror(errno));
367
			ast_log(LOG_ERROR, "Error reading from web socket: %s\n", strerror(errno));
339
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
368
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
340
			session->closing = 1;
369
			session->closing = 1;

    
   
370
			ao2_unlock(session);
341
			return -1;
371
			return -1;
342
		}
372
		}
343
		xlen = (xlen - rlen);
373
		xlen = (xlen - rlen);
344
		rbuf = rbuf + rlen;
374
		rbuf = rbuf + rlen;
345
		if (0 == xlen) {
375
		if (0 == xlen) {
346
			break;
376
			break;
347
		}
377
		}
348
		if (ast_wait_for_input(session->fd, 1000) < 0) {
378
		if (ast_wait_for_input(session->fd, 1000) < 0) {
349
			ast_log(LOG_ERROR, "ast_wait_for_input returned err: %s\n", strerror(errno));
379
			ast_log(LOG_ERROR, "ast_wait_for_input returned err: %s\n", strerror(errno));
350
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
380
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
351
			session->closing = 1;
381
			session->closing = 1;

    
   
382
			ao2_unlock(session);
352
			return -1;
383
			return -1;
353
		}
384
		}
354
	}
385
	}
355
	if (!sanity) {
386
	if (!sanity) {
356
		ast_log(LOG_WARNING, "Websocket seems unresponsive, disconnecting ...\n");
387
		ast_log(LOG_WARNING, "Websocket seems unresponsive, disconnecting ...\n");
357
		(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
388
		(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
358
		session->closing = 1;
389
		session->closing = 1;

    
   
390
		ao2_unlock(session);
359
		return -1;
391
		return -1;
360
	}
392
	}

    
   
393
	ao2_unlock(session);
361
	return 0;
394
	return 0;
362
}
395
}
363

    
   
396

   
364
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)
397
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)
365
{
398
{
[+20] [20] 64 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)
430
			for (pos = 0; pos < *payload_len; pos++) {
463
			for (pos = 0; pos < *payload_len; pos++) {
431
				(*payload)[pos] ^= mask[pos % 4];
464
				(*payload)[pos] ^= mask[pos % 4];
432
			}
465
			}
433
		}
466
		}
434

    
   
467

   

    
   
468
		ao2_lock(session);
435
		if (!(new_payload = ast_realloc(session->payload, (session->payload_len + *payload_len)))) {
469
		if (!(new_payload = ast_realloc(session->payload, (session->payload_len + *payload_len)))) {
436
			ast_log(LOG_WARNING, "Failed allocation: %p, %zd, %"PRIu64"\n",
470
			ast_log(LOG_WARNING, "Failed allocation: %p, %zd, %"PRIu64"\n",
437
				session->payload, session->payload_len, *payload_len);
471
				session->payload, session->payload_len, *payload_len);

    
   
472
			ao2_unlock(session);
438
			*payload_len = 0;
473
			*payload_len = 0;
439
			ast_websocket_close(session, 1009);
474
			ast_websocket_close(session, 1009);
440
			return 0;
475
			return 0;
441
		}
476
		}
442

    
   
477

   
[+20] [20] 26 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)
469
			}
504
			}
470
			*payload_len = session->payload_len;
505
			*payload_len = session->payload_len;
471
			*payload = session->payload;
506
			*payload = session->payload;
472
			session->payload_len = 0;
507
			session->payload_len = 0;
473
		}
508
		}

    
   
509
		ao2_unlock(session);
474
	} else if (*opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
510
	} else if (*opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
475
		/* Make the payload available so the user can look at the reason code if they so desire */
511
		/* Make the payload available so the user can look at the reason code if they so desire */

    
   
512
		ao2_lock(session);
476
		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
513
		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
477
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
514
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
478
				return 0;
515
				return 0;
479
			}
516
			}
480
			session->payload = new_payload;
517
			session->payload = new_payload;
[+20] [20] 7 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)
488
		}
525
		}
489

    
   
526

   
490
		fclose(session->f);
527
		fclose(session->f);
491
		session->f = NULL;
528
		session->f = NULL;
492
		ast_verb(2, "WebSocket connection from '%s' closed\n", ast_sockaddr_stringify(&session->address));
529
		ast_verb(2, "WebSocket connection from '%s' closed\n", ast_sockaddr_stringify(&session->address));

    
   
530
		ao2_unlock(session);
493
	} else {
531
	} else {
494
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
532
		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
533
		/* 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. */
534
		 * fit that, I think. */
497
		ast_websocket_close(session, 1003);
535
		ast_websocket_close(session, 1003);
[+20] [20] 138 lines
[+20] [+] static int websocket_callback(struct ast_tcptls_session_instance *ser, const struct ast_http_uri *urih, const char *uri, enum ast_http_method method, struct ast_variable *get_vars, struct ast_variable *headers)
636
	}
674
	}
637

    
   
675

   
638
	ast_verb(2, "WebSocket connection from '%s' for protocol '%s' accepted using version '%d'\n", ast_sockaddr_stringify(&ser->remote_address), protocol, version);
676
	ast_verb(2, "WebSocket connection from '%s' for protocol '%s' accepted using version '%d'\n", ast_sockaddr_stringify(&ser->remote_address), protocol, version);
639

    
   
677

   
640
	/* Populate the session with all the needed details */
678
	/* Populate the session with all the needed details */

    
   
679
	ao2_lock(session);
641
	session->f = ser->f;
680
	session->f = ser->f;
642
	session->fd = ser->fd;
681
	session->fd = ser->fd;
643
	ast_sockaddr_copy(&session->address, &ser->remote_address);
682
	ast_sockaddr_copy(&session->address, &ser->remote_address);
644
	session->opcode = -1;
683
	session->opcode = -1;
645
	session->reconstruct = DEFAULT_RECONSTRUCTION_CEILING;
684
	session->reconstruct = DEFAULT_RECONSTRUCTION_CEILING;
646
	session->secure = ser->ssl ? 1 : 0;
685
	session->secure = ser->ssl ? 1 : 0;

    
   
686
	ao2_unlock(session);
647

    
   
687

   
648
	/* Give up ownership of the socket and pass it to the protocol handler */
688
	/* Give up ownership of the socket and pass it to the protocol handler */
649
	protocol_handler->callback(session, get_vars, headers);
689
	protocol_handler->callback(session, get_vars, headers);
650
	ao2_ref(protocol_handler, -1);
690
	ao2_ref(protocol_handler, -1);
651

    
   
691

   
[+20] [20] 81 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.