845707 - [Buri][Music] Songs title with "~" can not be played.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[sync-1]

+++ This bug was initially created as a clone of Bug #410657 +++
 
 DEFECT DESCRIPTION:
 Songs title with "~" can not be played.
 
 REPRODUCING PROCEDURES:
 IF the song title contain  "~" ,it can not be played.--->KO
 If the directery name contain "~", same problem found.
 
 EXPECTED BEHAVIOUR:
 Can play normally.
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #410657 description ++++++++++

Comment 1

[Kai-Chih Hu [:khu]]

Dominic, could you help, or know who could help? Thanks.

Comment 3

[Jason Smith [:jsmith]]

This could potentially be a device storage bug. Including Doug just in case.

Comment 4

[Doug Turner (:dougt)]

Yes, we filter out ~.  the worry is a path like ~/

http://mxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#359

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

songs with "~" showed up in music app but cannot play music. Seem like a bug we need to fix.

dkuo/doug, any thoughts? thanks

Comment 6

[Kai-Chih Hu [:khu]]

I checked the codes and it looks like we could have better filter criteria to filter out user home directory.

Comment 7

[Doug Turner (:dougt)]

khu: yup.  what we are doing is not really great.

Comment 8

[Jason Smith [:jsmith]]

Quick question - is the bug here technically at the Gaia level, device storage level, or both?

Comment 9

[Dominic Kuo [:dkuo]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Quick question - is the bug here technically at the Gaia level, device
> storage level, or both?

This is an issue at device storage level I believe, cause when music player is trying to retrieve the audio file that starts with "~", that file does exist but device storage will return a SecurityError, and music player is unable to play that file.

Comment 10

[Alex Keybl [:akeybl]]

The size of the impacted user population will be so small that we can fix for the first time in v2.

Comment 11

[Andrew Overholt [:overholt]]

Can we just not show files with ~ in their name in the music player?

Comment 13

[Dave Hylands [:dhylands]]

I think that this is a DeviceStorage limititation.

Right now, device storage ignores all files with ~ in them. I think it should only ignore files with leading tildes in them.

The leading tilde thing is to prevent ~username from redirecting outside the filesystem.

Embedded tildes are common, especially on FAT filesystems. You'll often see files like SOMELO~1.png because it didn't fit in an 8.3 format.

Comment 14

[Dave Hylands [:dhylands]]

Personally, I think that tildes in the filenames, leading or otherwise, should be legal. The filesystem doesn't do ~ expansion, the shell does. device storage shouldn't be calling any APIs which do tilde expansion, so there shouldn't be any security issues.

Comment 15

[Wayne Chang [:wchang]]

Triage 4/12 - Leo- , this is nice to have but not blocking for release.

Comment 17

[leo.bugzilla.gecko]

This issue leads two problem on the device.
1. Video thumbnail is shown on video app but it cannot be played. (music and gallery application have same problem)
2. Since video application doesn't consider errors, device freezed after trying to play these kind of video clip.

So, the mimetype cannot be fixed, those application should not list up the files that have "~" character in its file name.

Comment 18

[Dylan Oliver [:doliver]]

This issue has been identified as a QE1 blocker. Per comment #13 and comment #14, it seems like we need better handling here.

Comment 19

[Dominic Kuo [:dkuo]]

Kanru,

can you help on this? thanks.

Comment 20

[Wayne Chang [:wchang]]

Triage 4/16 - Leo+ as blocking partner certification.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Dave Hylands [:dhylands] from comment #14)
> Personally, I think that tildes in the filenames, leading or otherwise,
> should be legal. The filesystem doesn't do ~ expansion, the shell does.
> device storage shouldn't be calling any APIs which do tilde expansion, so
> there shouldn't be any security issues.

Unfortunately the underlying nsLocalFileUnix implementation do expend the path if it begins with '~' or '~/'.

I think the correct way to handle this is to have a version that doesn't expand it.

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Only filter file path that is a '~' or starts with '~/'

Comment 23

[Dave Hylands [:dhylands]]

(In reply to Kan-Ru Chen [:kanru] from comment #21)
> (In reply to Dave Hylands [:dhylands] from comment #14)
> > Personally, I think that tildes in the filenames, leading or otherwise,
> > should be legal. The filesystem doesn't do ~ expansion, the shell does.
> > device storage shouldn't be calling any APIs which do tilde expansion, so
> > there shouldn't be any security issues.
> 
> Unfortunately the underlying nsLocalFileUnix implementation do expend the
> path if it begins with '~' or '~/'.
> 
> I think the correct way to handle this is to have a version that doesn't
> expand it.

Well we should still never be creating an nsLocalFileUnix using the pathname from deviceStorageFile.

We should always be creating the pathname by concatenating the root directory with deviceStorageFile path.

We should make sure that the root portion is never empty, or if it is make it be ./

Then there would never be any ~ expansion.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

It seems we always set a root directory for each deviceStorage. The check is relatively fast so I want to keep it and output a warning if we get a path name that starts with '~'.

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 845707 - Only filter file path that is a '~' or starts with '~/'

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 738393 [details] [diff] [review]
Bug 845707 - Only filter file path that is a '~' or starts with '~/'

Review of attachment 738393 [details] [diff] [review]:
-----------------------------------------------------------------

i think we care less about other platforms now.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/98c192813bba

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98c192813bba

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7d7733dc41e4