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.

 

Changes between revision 1 and 2

1 2 3 4
1 2 3 4

  1. branches/11/res/res_http_websocket.c: Loading...
branches/11/res/res_http_websocket.c
Diff Revision 1 Diff Revision 2
[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] 60 lines
[+20] [+] int AST_OPTIONAL_API_NAME(ast_websocket_remove_protocol)(const char *name, ast_websocket_callback callback)
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 */
191
	int res;
193
	int res;
192

    
   
194

   

    
   
195
	if (session->close_sent) {

    
   
196
		return 0;

    
   
197
	}

    
   
198

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

    
   
201

   
196
	/* 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 */
197
	put_unaligned_uint16(&frame[2], htons(reason ? reason : 1000));
203
	put_unaligned_uint16(&frame[2], htons(reason ? reason : 1000));
198

    
   
204

   
199
	ao2_lock(session);

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

    
   
206
	session->close_sent = 1;
201

    
   
207

   

    
   
208
	ao2_lock(session);
202
	res = (fwrite(frame, 1, 4, session->f) == 4) ? 0 : -1;
209
	res = (fwrite(frame, 1, 4, session->f) == 4) ? 0 : -1;
203
	ao2_unlock(session);
210
	ao2_unlock(session);
204
	return res;
211
	return res;
205
}
212
}
206

    
   
213

   
[+20] [20] 29 lines
[+20] [+] int AST_OPTIONAL_API_NAME(ast_websocket_write)(struct ast_websocket *session, enum ast_websocket_opcode opcode, char *payload, uint64_t actual_length)
236
	} else if (length == 127) {
243
	} else if (length == 127) {
237
		put_unaligned_uint64(&frame[2], htonl(actual_length));
244
		put_unaligned_uint64(&frame[2], htonl(actual_length));
238
	}
245
	}
239

    
   
246

   
240
	ao2_lock(session);
247
	ao2_lock(session);

    
   
248
	if (session->closing) {

    
   
249
		ao2_unlock(session);

    
   
250
		return -1;

    
   
251
	}

    
   
252

   
241
	if (fwrite(frame, 1, header_size, session->f) != header_size) {
253
	if (fwrite(frame, 1, header_size, session->f) != header_size) {
242
		ao2_unlock(session);
254
		ao2_unlock(session);
243
		return -1;
255
		return -1;
244
	}
256
	}
245

    
   
257

   
[+20] [20] 7 lines
[+20] int AST_OPTIONAL_API_NAME(ast_websocket_write)(struct ast_websocket *session, enum ast_websocket_opcode opcode, char *payload, uint64_t actual_length)
253
	return 0;
265
	return 0;
254
}
266
}
255

    
   
267

   
256
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)
257
{
269
{
258
	ao2_lock(session);

   
259
	session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);
270
	session->reconstruct = MIN(bytes, MAXIMUM_RECONSTRUCTION_CEILING);
260
	ao2_unlock(session);

   
261
}
271
}
262

    
   
272

   
263
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_disable)(struct ast_websocket *session)
273
void AST_OPTIONAL_API_NAME(ast_websocket_reconstruct_disable)(struct ast_websocket *session)
264
{
274
{
265
	ao2_lock(session);

   
266
	session->reconstruct = 0;
275
	session->reconstruct = 0;
267
	ao2_unlock(session);

   
268
}
276
}
269

    
   
277

   
270
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
278
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
271
{
279
{
272
	ao2_ref(session, +1);
280
	ao2_ref(session, +1);
[+20] [20] 4 lines
[+20] [+] void AST_OPTIONAL_API_NAME(ast_websocket_unref)(struct ast_websocket *session)
277
	ao2_ref(session, -1);
285
	ao2_ref(session, -1);
278
}
286
}
279

    
   
287

   
280
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
288
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
281
{
289
{
282
	int res;
290
	return session->closing ? -1 : session->fd;
283
	ao2_lock(session);

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

   
285
	ao2_unlock(session);

   
286
	return res;

   
287
}
291
}
288

    
   
292

   
289
struct ast_sockaddr * AST_OPTIONAL_API_NAME(ast_websocket_remote_address)(struct ast_websocket *session)
293
struct ast_sockaddr * AST_OPTIONAL_API_NAME(ast_websocket_remote_address)(struct ast_websocket *session)
290
{
294
{
291
	struct ast_sockaddr *res;
295
	return &session->address;
292
	ao2_lock(session);

   
293
	res = &session->address;

   
294
	ao2_unlock(session);

   
295
	return res;

   
296
}
296
}
297

    
   
297

   
298
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)
299
{
299
{
300
	int res;
300
	return session->secure;
301
	ao2_lock(session);

   
302
	res = session->secure;

   
303
	ao2_unlock(session);

   
304
	return res;

   
305
}
301
}
306

    
   
302

   
307
int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
303
int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
308
{
304
{
309
	int flags;
305
	int flags;
310

    
   
306

   
311
	ao2_lock(session);

   
312
	if ((flags = fcntl(session->fd, F_GETFL)) == -1) {
307
	if ((flags = fcntl(session->fd, F_GETFL)) == -1) {
313
		ao2_unlock(session);

   
314
		return -1;
308
		return -1;
315
	}
309
	}
316

    
   
310

   
317
	flags |= O_NONBLOCK;
311
	flags |= O_NONBLOCK;
318

    
   
312

   
319
	if ((flags = fcntl(session->fd, F_SETFL, flags)) == -1) {
313
	if ((flags = fcntl(session->fd, F_SETFL, flags)) == -1) {
320
		ao2_unlock(session);

   
321
		return -1;
314
		return -1;
322
	}
315
	}
323
	ao2_unlock(session);

   
324

    
   
316

   
325
	return 0;
317
	return 0;
326
}
318
}
327

    
   
319

   
328
/* MAINTENANCE WARNING on ast_websocket_read()!
320
/* MAINTENANCE WARNING on ast_websocket_read()!
[+20] [20] 28 lines
[+20] int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
357
{
349
{
358
	int sanity;
350
	int sanity;
359
	size_t rlen;
351
	size_t rlen;
360
	int xlen = len;
352
	int xlen = len;
361
	char *rbuf = buf;
353
	char *rbuf = buf;
362
	ao2_lock(session);

   
363
	for (sanity = 10; sanity; sanity--) {
354
	for (sanity = 10; sanity; sanity--) {
364
		clearerr(session->f);
355
		clearerr(session->f);
365
		rlen = fread(rbuf, 1, xlen, session->f);
356
		rlen = fread(rbuf, 1, xlen, session->f);
366
		if (0 == rlen && ferror(session->f) && errno != EAGAIN) {
357
		if (0 == rlen && ferror(session->f) && errno != EAGAIN) {
367
			ast_log(LOG_ERROR, "Error reading from web socket: %s\n", strerror(errno));
358
			ast_log(LOG_ERROR, "Error reading from web socket: %s\n", strerror(errno));
368
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
359
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
369
			session->closing = 1;
360
			session->closing = 1;
370
			ao2_unlock(session);

   
371
			return -1;
361
			return -1;
372
		}
362
		}
373
		xlen = (xlen - rlen);
363
		xlen = (xlen - rlen);
374
		rbuf = rbuf + rlen;
364
		rbuf = rbuf + rlen;
375
		if (0 == xlen) {
365
		if (0 == xlen) {
376
			break;
366
			break;
377
		}
367
		}
378
		if (ast_wait_for_input(session->fd, 1000) < 0) {
368
		if (ast_wait_for_input(session->fd, 1000) < 0) {
379
			ast_log(LOG_ERROR, "ast_wait_for_input returned err: %s\n", strerror(errno));
369
			ast_log(LOG_ERROR, "ast_wait_for_input returned err: %s\n", strerror(errno));
380
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
370
			(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
381
			session->closing = 1;
371
			session->closing = 1;
382
			ao2_unlock(session);

   
383
			return -1;
372
			return -1;
384
		}
373
		}
385
	}
374
	}
386
	if (!sanity) {
375
	if (!sanity) {
387
		ast_log(LOG_WARNING, "Websocket seems unresponsive, disconnecting ...\n");
376
		ast_log(LOG_WARNING, "Websocket seems unresponsive, disconnecting ...\n");
388
		(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
377
		(*opcode) = AST_WEBSOCKET_OPCODE_CLOSE;
389
		session->closing = 1;
378
		session->closing = 1;
390
		ao2_unlock(session);

   
391
		return -1;
379
		return -1;
392
	}
380
	}
393
	ao2_unlock(session);

   
394
	return 0;
381
	return 0;
395
}
382
}
396

    
   
383

   
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)
384
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)
398
{
385
{
[+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)
463
			for (pos = 0; pos < *payload_len; pos++) {
450
			for (pos = 0; pos < *payload_len; pos++) {
464
				(*payload)[pos] ^= mask[pos % 4];
451
				(*payload)[pos] ^= mask[pos % 4];
465
			}
452
			}
466
		}
453
		}
467

    
   
454

   
468
		ao2_lock(session);

   
469
		if (!(new_payload = ast_realloc(session->payload, (session->payload_len + *payload_len)))) {
455
		if (!(new_payload = ast_realloc(session->payload, (session->payload_len + *payload_len)))) {
470
			ast_log(LOG_WARNING, "Failed allocation: %p, %zd, %"PRIu64"\n",
456
			ast_log(LOG_WARNING, "Failed allocation: %p, %zd, %"PRIu64"\n",
471
				session->payload, session->payload_len, *payload_len);
457
				session->payload, session->payload_len, *payload_len);
472
			ao2_unlock(session);

   
473
			*payload_len = 0;
458
			*payload_len = 0;
474
			ast_websocket_close(session, 1009);
459
			ast_websocket_close(session, 1009);
475
			return 0;
460
			return 0;
476
		}
461
		}
477

    
   
462

   
[+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)
504
			}
489
			}
505
			*payload_len = session->payload_len;
490
			*payload_len = session->payload_len;
506
			*payload = session->payload;
491
			*payload = session->payload;
507
			session->payload_len = 0;
492
			session->payload_len = 0;
508
		}
493
		}
509
		ao2_unlock(session);

   
510
	} else if (*opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
494
	} else if (*opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
511
		/* Make the payload available so the user can look at the reason code if they so desire */
495
		/* Make the payload available so the user can look at the reason code if they so desire */
512
		ao2_lock(session);

   
513
		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
496
		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
514
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
497
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
515
				return 0;
498
				return 0;
516
			}
499
			}
517
			session->payload = new_payload;
500
			session->payload = new_payload;
518
			memcpy(session->payload, &buf[frame_size], *payload_len);
501
			memcpy(session->payload, &buf[frame_size], *payload_len);
519
			*payload = session->payload;
502
			*payload = session->payload;
520
			frame_size += (*payload_len);
503
			frame_size += (*payload_len);
521
		}
504
		}
522

    
   
505

   
523
		if (!session->closing) {
506
		session->closing = 1;
524
			ast_websocket_close(session, 0);

   
525
		}

   
526

    
   

   
527
		fclose(session->f);

   
528
		session->f = NULL;

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

   
531
	} else {
508
	} else {
532
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
509
		ast_log(LOG_WARNING, "WebSocket unknown opcode %d\n", *opcode);
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
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
534
		 * fit that, I think. */
511
		 * fit that, I think. */
535
		ast_websocket_close(session, 1003);
512
		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)
674
	}
651
	}
675

    
   
652

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

    
   
654

   
678
	/* Populate the session with all the needed details */
655
	/* Populate the session with all the needed details */
679
	ao2_lock(session);

   
680
	session->f = ser->f;
656
	session->f = ser->f;
681
	session->fd = ser->fd;
657
	session->fd = ser->fd;
682
	ast_sockaddr_copy(&session->address, &ser->remote_address);
658
	ast_sockaddr_copy(&session->address, &ser->remote_address);
683
	session->opcode = -1;
659
	session->opcode = -1;
684
	session->reconstruct = DEFAULT_RECONSTRUCTION_CEILING;
660
	session->reconstruct = DEFAULT_RECONSTRUCTION_CEILING;
685
	session->secure = ser->ssl ? 1 : 0;
661
	session->secure = ser->ssl ? 1 : 0;
686
	ao2_unlock(session);

   
687

    
   
662

   
688
	/* Give up ownership of the socket and pass it to the protocol handler */
663
	/* Give up ownership of the socket and pass it to the protocol handler */
689
	protocol_handler->callback(session, get_vars, headers);
664
	protocol_handler->callback(session, get_vars, headers);
690
	ao2_ref(protocol_handler, -1);
665
	ao2_ref(protocol_handler, -1);
691

    
   
666

   
[+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.