[PATCH BlueZ 0/5] player: Fix crash and related defects around the pending request

George Kiagiadakis <[email protected]>
Newsgroups org.kernel.vger.linux-bluetooth
Message-ID <[email protected]>
A player that advertises the AVRCP NowPlaying feature bit but not the
Browsing bit exports playable MediaItem1 objects while the player scope
is still unset, and calling org.bluez.MediaItem1.Play() on one of them
crashes bluetoothd with a NULL dereference at offset 0x28.

media_item_play() dereferences mp->scope, which is only ever set from
SetBrowsedPlayer or ChangeFolder, and both are reached only when the
player advertises Browsing (features[7] & 0x08). The /NowPlaying folder
and its playable items are gated on a different bit (features[8] &
0x02), so the two can disagree. msg sits at offset 40 in struct
media_folder on LP64, which is the reported fault address.

Patch 1 fixes the crash. A plain NULL check is not enough, because the
pending message would then have nowhere to live and Play() would answer
nothing at all rather than crash. The pending request moves from struct
media_folder to struct media_player instead. Every user already stored
into mp->scope->msg, so the slot was per player in all but name, and the
mutual exclusion between ListItems, Search, ChangeFolder and Play is
preserved. The move also fixes a lost reply, since each completion
re-read mp->scope and found a different folder whenever avrcp moved the
scope while a request was in flight.

Patches 2 to 4 are further defects in the same area, found while
auditing the ownership of that message:

  - destroying a player dropped the pending request without answering
    it, so the caller waited out its D-Bus timeout on every AVRCP
    disconnect;

  - NumberOfItems has not been refreshed on SetBrowsedPlayer since
    f17d3a2c3, because a guard swallows the property update that
    commit deferred into the completion;

  - a busy Search() reports EINVAL where its three siblings report
    EBUSY.

Patch 5 adds unit/test-media-player, which drives the D-Bus surface of
profiles/audio/player.c over a private session bus. Against the tree
before this series, three of its nine tests crash and three fail.

Each patch builds and passes make check on its own. The final tree
passes 39/39 and is clean under valgrind.

George Kiagiadakis (5):
  player: Fix crash on MediaItem1.Play() without a browsing scope
  player: Answer pending request when the player is destroyed
  player: Fix NumberOfItems never being updated on SetBrowsedPlayer
  player: Report EBUSY from a busy Search()
  unit/test-media-player: Add media player tests

 .gitignore               |   1 +
 Makefile.am              |  13 +
 profiles/audio/player.c  |  80 ++--
 unit/test-media-player.c | 838 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 891 insertions(+), 41 deletions(-)
 create mode 100644 unit/test-media-player.c


base-commit: c73fa2f9ae2d366cb8a4f101fa9a5ccd9f33a4ea
-- 
2.54.0 (Apple Git-157)
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.