This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#12500 - /./ is invalid when not using dircache, so HOME_DIR breaks things
Attached to Project:
Rockbox
Opened by Boris Gjenero (dreamlayers) - Saturday, 31 December 2011, 18:57 GMT+2
Last edited by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 01:43 GMT+2
Opened by Boris Gjenero (dreamlayers) - Saturday, 31 December 2011, 18:57 GMT+2
Last edited by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 01:43 GMT+2
|
DetailsWhen Rockbox is directly accessing the disk, paths starting with /./ are not valid. The FAT32 root directory does not have the . and .. entries found in other directories. It is possible to use . and .. in other directories, even for getting to the root directory from a 1st level directory.
This causes HOME_DIR, introduced in r31430, to break some things. It seems dircache can deal with it. However, file creation always uses opendir_uncached(), which fails. Here are two examples: When recording to the default recording directory, the path is "/./filename.ext". With dircache disabled, the recording screen can't be entered, because "/./" can't be accessed. With dircache enabled, recording starts, but saving fails as if the disk is full. When saving a playlist to the playlist catalog, the path is "/./Playlists/name.m3u8". With dircache disabled, I can't even enter a filename because the path is inaccessible. With dircache enabled, I can edit the filename and remove the /. at the start to make it work. If I don't remove the /. then there is no error message but the playlist fails to save. This lack of error message is another bug. I tested this on my 5G 30GB iPod running r31467. |
This task depends upon
Closed by Boris Gjenero (dreamlayers)
Friday, 20 January 2012, 01:43 GMT+2
Reason for closing: Fixed
Additional comments about closing: HOME_DIR issue fixed in 6e11289. There doesn't seem to be much interest in adding support for "." and "..", and I'm not even convinced that it's a good thing to add.
Friday, 20 January 2012, 01:43 GMT+2
Reason for closing: Fixed
Additional comments about closing: HOME_DIR issue fixed in 6e11289. There doesn't seem to be much interest in adding support for "." and "..", and I'm not even convinced that it's a good thing to add.
If so, we should probably fix it. Otherwise we can undo that particular change and go back to HOME_DIR == "/" which is not entirely right IIUC (it can create paths starting with //). But that was before and worked, although amiconn noted that cygwin interprets // paths as network locations.
I don't like how /. is visible to the user, and how it uses up screen and memory space. Because of this concern, I was thinking about making HOME_DIR blank. The problem there is that opendir(HOME_DIR) won't work. This would currently only be a problem in create_numbered_filename(). It doesn't seem like there's any simple and elegant solution for removing the prefix.
Edit: With opendir_uncached_recognize_dot.patch, /./<volumename>/ is still broken, because it goes to the root file system instead of the volume. In strip_volume(), repeated / at the start are handled, but /./ isn't. Since strip_volume() is also used in open_internal(), it would have to be changed to handle this. I guess it doesn't matter though, because "/./<volumename>/" wouldn't happen. BTW Also .. is broken in connection with multivolume.
It may be best to use "/" for HOME_DIR. Rockbox can already handle //, and I don't see why Windows would ever be given paths starting with //.
the patch solve the problem.
Passing "/" to create_*_filename() works, though it creates filenames starting with "//". Recording replaces "/" with "" to avoid this. BTW This makes me think the following change in r31430 in create_numbered_filename() is wrong:
- dir = opendir(pathlen ? buffer : "/");
+ dir = opendir(pathlen ? buffer : HOME_DIR);
The code was probably there because passing "" as path is the proper way to create a filename in the root directory. Also, in r31430 the caller passes HOME_DIR, so either the defaulting to HOME_DIR or the passing of HOME_DIR is redundant.
It would be easy to change create_*_filename() to not create paths with "//" in the result. However, not much is gained as long as the preprocessor elsewhere creates paths starting with "//".
Would it be okay to have HOME_DIR end with a slash instead of appended things starting with a slash? This will certainly work if something is being appended. Also, our directory functions should work because strtok_r() will cause trailing slashes to be ignored. I just don't know if any other operating system requires that paths passed to directory functions not have a trailing slash.
I'm attaching code for more fully supporting "." and "..". Remaining issues and limitations are described below. Note that it's possible to test HAVE_MULTIVOLUME by shrinking the main FAT32 partition and creating another partition.
In opendir_uncached(), the parent directory passed to fat_opendir() is wrong after "..". It only seems to cause a problem in an unlikely rmdir() race condition. For more info, read the FIXME comment.
The _dircache_get_entry_volume() function nicely matches the API, but it does an extra dircache traverse from the entry toward the root. I think it's ok because the traverse is cheap. Otherwise, dircache_get_entry() would have to be changed to return the volume.
I did not fix strip_volume(). It's now only used in generate_bookmark_file_name().
In playlist_save(), strncmp() is used to check if the current playlist is being overwritten. That will fail if the playlist is being overwritten using a different path. I did not search for similar path comparisons elsewhere.
The last two could be solved by a function which creates a canonical file name, similar to POSIX realpath(). Adding more disk access seems like a bad idea, so it should probably be done via string manipulation. This could also replace the dircache changes. Accepting "/nonexistent/path/../../real/path/" is okay, because it would fail elsewhere. If succeeding with such paths is okay, then such a function could even replace the opendir_uncached() changes.
Full support for . and .. would make it possible to simplify format_track_path() in playlist code.