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 4

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 4
[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] 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)
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 */
191
	int res;
194
	int res;
192

    
   
195

   

    
   
196
	if (session->close_sent) {

    
   
197
		return 0;

    
   
198
	}

    
   
199

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

    
   
202

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

    
   
205

   
199
	ao2_lock(session);

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

    
   
207
	session->close_sent = 1;
201

    
   
208

   

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

    
   
214

   
[+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) {
244
	} else if (length == 127) {
237
		put_unaligned_uint64(&frame[2], htonl(actual_length));
245
		put_unaligned_uint64(&frame[2], htonl(actual_length));
238
	}
246
	}
239

    
   
247

   
240
	ao2_lock(session);
248
	ao2_lock(session);

    
   
249
	if (session->closing) {

    
   
250
		ao2_unlock(session);

    
   
251
		return -1;

    
   
252
	}

    
   
253

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

    
   
258

   
[+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;
266
	return 0;
254
}
267
}
255

    
   
268

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

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

   
261
}
272
}
262

    
   
273

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

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

   
268
}
277
}
269

    
   
278

   
270
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
279
void AST_OPTIONAL_API_NAME(ast_websocket_ref)(struct ast_websocket *session)
271
{
280
{
272
	ao2_ref(session, +1);
281
	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);
286
	ao2_ref(session, -1);
278
}
287
}
279

    
   
288

   
280
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
289
int AST_OPTIONAL_API_NAME(ast_websocket_fd)(struct ast_websocket *session)
281
{
290
{
282
	int res;
291
	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
}
292
}
288

    
   
293

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

   
293
	res = &session->address;

   
294
	ao2_unlock(session);

   
295
	return res;

   
296
}
297
}
297

    
   
298

   
298
int AST_OPTIONAL_API_NAME(ast_websocket_is_secure)(struct ast_websocket *session)
299
int AST_OPTIONAL_API_NAME(ast_websocket_is_secure)(struct ast_websocket *session)
299
{
300
{
300
	int res;
301
	return session->secure;
301
	ao2_lock(session);

   
302
	res = session->secure;

   
303
	ao2_unlock(session);

   
304
	return res;

   
305
}
302
}
306

    
   
303

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

    
   
307

   
311
	ao2_lock(session);

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

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

    
   
311

   
317
	flags |= O_NONBLOCK;
312
	flags |= O_NONBLOCK;
318

    
   
313

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

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

   
324

    
   
317

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

    
   
320

   
328
/* MAINTENANCE WARNING on ast_websocket_read()!
321
/* 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
{
350
{
358
	int sanity;
351
	int sanity;
359
	size_t rlen;
352
	size_t rlen;
360
	int xlen = len;
353
	int xlen = len;
361
	char *rbuf = buf;
354
	char *rbuf = buf;
362
	ao2_lock(session);

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

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

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

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

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

    
   
384

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

    
   
455

   
468
		ao2_lock(session);

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

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

    
   
463

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

   
510
	} else if (*opcode == AST_WEBSOCKET_OPCODE_CLOSE) {
495
	} 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 */
496
		/* 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))) {
497
		if ((*payload_len) && (new_payload = ast_realloc(session->payload, *payload_len))) {
514
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
498
			if (ws_safe_read(session, &buf[frame_size], (*payload_len), opcode)) {
515
				return 0;
499
				return 0;
516
			}
500
			}
517
			session->payload = new_payload;
501
			session->payload = new_payload;
518
			memcpy(session->payload, &buf[frame_size], *payload_len);
502
			memcpy(session->payload, &buf[frame_size], *payload_len);
519
			*payload = session->payload;
503
			*payload = session->payload;
520
			frame_size += (*payload_len);
504
			frame_size += (*payload_len);
521
		}
505
		}
522

    
   
506

   
523
		if (!session->closing) {
507
		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));

   
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.