Audio: fix race condition when starting a stream

PATCH Applied Peter Hurley 2 participants 2 messages Jul 7, 2011 – Jul 10, 2011 Bluetooth drivers

Share & tools

Peter Hurley

author

2011-07-07 14:51 UTC #0 lore raw

PATCH 1/1 Audio: fix race condition when starting a stream

The AVDTP spec allows for a race condition between remote and local device when issuing an AVDTP_START cmd on a stream in the OPEN state.

However, the internal state must continue to be consistent. For example, suppose that avdtp_start() has been called while in the OPEN state and a AVDTP_START cmd is sent. Now before we have received a response (and thus entered the STREAMING state), we *receive* a START cmd. Prior to this fix, since the sep is still in the OPEN state, we would accept the new START cmd. This will leads us to send both a Start_Ind and Start_Cfm - not good.

Now, we track this transitional state (starting == TRUE).

NB - 'starting' is only in a valid state while the sep is in the OPEN state. 'starting' is reset when we return to the OPEN state.

---
 audio/avdtp.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

audio/avdtp.c +25 -4

index 44df636..c12f5a3 100644
@@ -378,6 +378,7 @@ struct avdtp_stream {
 	guint idle_timer;
 	gboolean delay_reporting;
 	uint16_t delay;		/* AVDTP 1.3 Delay Reporting feature */
+	gboolean starting;	/* only valid while sep state == OPEN */
 };
 
 /* Structure describing an AVDTP connection between two devices */

@@ -1068,6 +1069,7 @@ static void avdtp_sep_set_state(struct avdtp *session,
 			avdtp_delay_report(session, stream, stream->delay);
 		break;
 	case AVDTP_STATE_OPEN:
+		stream->starting = FALSE;
 		if (old_state > AVDTP_STATE_OPEN && session->auto_dc)
 			stream->idle_timer = g_timeout_add_seconds(STREAM_TIMEOUT,
 								stream_timeout,

@@ -1712,10 +1714,13 @@ static gboolean avdtp_start_cmd(struct avdtp *session, uint8_t transaction,
 
 		stream = sep->stream;
 
-		if (sep->state != AVDTP_STATE_OPEN) {
+		/* Also reject start cmd if we already initiated start */
+		if (sep->state != AVDTP_STATE_OPEN ||
+				stream->starting == TRUE) {
 			err = AVDTP_BAD_STATE;
 			goto failed;
 		}
+		stream->starting = TRUE;
 
 		if (sep->ind && sep->ind->start) {
 			if (!sep->ind->start(session, sep, stream, &err,

@@ -1730,6 +1735,7 @@ static gboolean avdtp_start_cmd(struct avdtp *session, uint8_t transaction,
 						AVDTP_START, NULL, 0);
 
 failed:
+	DBG("Rejecting (%d)", err);
 	memset(&rej, 0, sizeof(rej));
 	rej.acp_seid = failed_seid;
 	rej.error = err;

@@ -2584,9 +2590,12 @@ static int cancel_request(struct avdtp *session, int err)
 		break;
 	case AVDTP_START:
 		error("Start: %s (%d)", strerror(err), err);
-		if (lsep && lsep->cfm && lsep->cfm->start)
+		if (lsep && lsep->cfm && lsep->cfm->start) {
 			lsep->cfm->start(session, lsep, stream, &averr,
 						lsep->user_data);
+			if (stream)
+				stream->starting = FALSE;
+		}
 		break;
 	case AVDTP_SUSPEND:
 		error("Suspend: %s (%d)", strerror(err), err);

@@ -3092,9 +3101,11 @@ static gboolean avdtp_parse_rej(struct avdtp *session,
 			return FALSE;
 		error("START request rejected: %s (%d)",
 				avdtp_strerror(&err), err.err.error_code);
-		if (sep && sep->cfm && sep->cfm->start)
+		if (sep && sep->cfm && sep->cfm->start) {
 			sep->cfm->start(session, sep, stream, &err,
 					sep->user_data);
+			stream->starting = FALSE;
+		}
 		return TRUE;
 	case AVDTP_SUSPEND:
 		if (!stream_rej_to_err(buf, size, &err, &acp_seid))

@@ -3552,6 +3563,7 @@ int avdtp_open(struct avdtp *session, struct avdtp_stream *stream)
 int avdtp_start(struct avdtp *session, struct avdtp_stream *stream)
 {
 	struct start_req req;
+	int ret;
 
 	if (!g_slist_find(session->streams, stream))
 		return -EINVAL;

@@ -3564,11 +3576,20 @@ int avdtp_start(struct avdtp *session, struct avdtp_stream *stream)
 		return -EINVAL;
 	}
 
+	if (stream->starting == TRUE) {
+		DBG("stream already started");
+		return -EINVAL;
+	}
+
 	memset(&req, 0, sizeof(req));
 	req.first_seid.seid = stream->rseid;
 
-	return send_request(session, FALSE, stream, AVDTP_START,
+	ret = send_request(session, FALSE, stream, AVDTP_START,
 							&req, sizeof(req));
+	if (ret == 0)
+		stream->starting = TRUE;
+
+	return ret;
 }
 
 int avdtp_close(struct avdtp *session, struct avdtp_stream *stream,

Signature · 1 line

1.7.4.1

Johan Hedberg

Applied

2011-07-10 07:55 UTC #1 lore raw

Hi Peter,

Quoted 11 lines · Peter Hurley

The AVDTP spec allows for a race condition between remote and local device when issuing an AVDTP_START cmd on a stream in the OPEN state.
However, the internal state must continue to be consistent. For example, suppose that avdtp_start() has been called while in the OPEN state and a AVDTP_START cmd is sent. Now before we have received a response (and thus entered the STREAMING state), we *receive* a START cmd. Prior to this fix, since the sep is still in the OPEN state, we would accept the new START cmd. This will leads us to send both a Start_Ind and Start_Cfm - not good.

Quoted 7 lines

Now, we track this transitional state (starting == TRUE).
NB - 'starting' is only in a valid state while the sep is in the OPEN state. 'starting' is reset when we return to the OPEN state.
---
 audio/avdtp.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

Applied. Thanks.

Johan