Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200619183456.38696-1-tedd.an@linux.intel.com>
In-Reply-To: <20200619183456.38696-1-tedd.an@linux.intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 22 Jun 2020 12:28:54 -0700
Message-ID: <CABBYNZLm9dWkcJ8Kbf=jeyN2VTzuRh2RFvY7iAqeqQ8LwSJ1cA@mail.gmail.com>
Subject: Re: [PATCH V2 1/2] midi: Fix SysEx parser in MIDI plugin
To:     Tedd Ho-Jeong An <tedd.an@linux.intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Tedd Ho-Jeong An <tedd.an@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Tedd,

On Fri, Jun 19, 2020 at 9:27 PM <tedd.an@linux.intel.com> wrote:
>
> From: Tedd Ho-Jeong An <tedd.an@intel.com>
>
> The SysEx end of message parser didn't consider the fact that
> timestampsLow are in the stream and it might have the EOX (F7)
> byte as well.
>
> Fix that by always assuming the first system message byte is
> the timestampLow.
>
> Future work would involve support other type of system message
> bytes, such as real-time.
> ---
>  profiles/midi/libmidi.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/profiles/midi/libmidi.c b/profiles/midi/libmidi.c
> index 4b4df799f..7d57e7335 100644
> --- a/profiles/midi/libmidi.c
> +++ b/profiles/midi/libmidi.c
> @@ -331,6 +331,30 @@ static size_t handle_end_of_sysex(struct midi_read_parser *parser,
>         return sysex_length + 1; /* +1 because of timestampLow */
>  }
>
> +/* Searches the end of a SysEx message that contains a timestampLow
> + * before the SysEx end byte. Returns the number of bytes of valid
> + * SysEx payload in the buffer.
> + */
> +static size_t sysex_eox_len(const uint8_t *data, size_t size)
> +{
> +       size_t i = 0;
> +
> +       MIDI_ASSERT(size > 0);
> +
> +       if (data[i] == 0xF0)
> +               i++;
> +
> +       /* Search for timestamp low */
> +       while (i < size) {
> +               if ((data[i] & 0x80)) {
> +                       i++;
> +                       break;
> +               }
> +               i++;
> +       }
> +
> +       return (i < size && data[i] == 0xF7) ? i : 0;
> +}
>
>
>  size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,
> @@ -368,14 +392,14 @@ size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,
>
>                 /* System Common Messages */
>         case 0xF0: /* SysEx Start */ {
> -               uint8_t *pos;
> +               size_t sysex_length;
>
>                 /* cleanup Running Status Message */
>                 parser->rstatus = 0;
>
> -               /* Avoid parsing if SysEx is contained in one BLE packet */
> -               if ((pos = memchr(data + i, 0xF7, size - i))) {
> -                       const size_t sysex_length = pos - (data + i);
> +               sysex_length = sysex_eox_len(data + i, size - i);
> +               /* Search for End of SysEx message in one BLE message */
> +               if (sysex_length > 0) {
>                         midi_size = handle_end_of_sysex(parser, ev, data + i,
>                                                         sysex_length);
>                 } else {
> @@ -424,10 +448,10 @@ size_t midi_read_raw(struct midi_read_parser *parser, const uint8_t *data,
>
>                 /* Check for SysEx messages */
>                 if (parser->sysex_stream.len > 0) {
> -                       uint8_t *pos;
> +                       size_t sysex_length;
>
> -                       if ((pos = memchr(data + i, 0xF7, size - i))) {
> -                               const size_t sysex_length = pos - (data + i);
> +                       sysex_length = sysex_eox_len(data + i, size - i);
> +                       if (sysex_length > 0) {
>                                 midi_size = handle_end_of_sysex(parser, ev, data + i,
>                                                                 sysex_length);
>                         } else {
> --
> 2.25.4
>

Applied, thanks.

-- 
Luiz Augusto von Dentz