GNOME Bugzilla – Bug 755608
Labeling fat16/fat32 partitions hangs if certain characters included in label
Last modified: 2015-10-27 17:05:53 UTC
Created attachment 312103 [details] Label-setting hang Setting a label for a fat16 or fat32 partition which contains certain characters (* ? . , ; : / \ | + = < > [ ]) causes gparted to hang by displaying a loading bar forever. The problem appears to be that mlabel will prompt for input (never given) if the label contains illegal characters. "Force Cancel" doesn't do anything; the only way to quit is to manually kill mlabel.
I was able to reproduce this error. A simple fix would be to strip any illegal characters in fat16.cc before it is sent to mlabel. I'll work on a patch for that tomorrow night.
Created attachment 312337 [details] [review] Patch to remove prohibited FAT16/32 Characters This patch removes prohibited characters from fat16/32 labels.
Thank you Max for reporting this and thank you Albert for writing a fix. The definitive information for what characters are allowed in FAT volume labels is: Microsoft TechNet: Label https://technet.microsoft.com/en-us/library/bb490925.aspx Replicated in Wikikedia: label (command) https://en.wikipedia.org/wiki/Label_%28command%29 Albert, When testing I found a few issues: 1) It is stripping space characters from the label, yet they are explicitly allowed according to the above linked information. 2) File system creation still allows creating labels with the prohibited characters. Please put the character stripping code into pad_label() and rename it to something like: sanitise_label(), prepare_label(), or similar. Also why is the character stripping done using std::string rather than std::ustring variables? Thanks, Mike
Created attachment 312392 [details] [review] Update to FAT16/32 label sanitation Mike, I've attached a new patch that corrects the issues you raised. I switched from string to ustring. Because I could not figure out how to get std::remove to work with the ustring, I broke the sanitation into two simple loops. --Albert
Created attachment 312457 [details] [review] Update to FAT16/32 label sanitation I realized after I submitted the previous patch that I had not placed the code into another function as requested. Sorry about that. This patch corrects the character sanitation issue and encapsulates the code into its own function.
It will likely be this weekend before I can look at this. Work is intruding into spare time for the rest of the week.
Thank you Albert for the updated patch, and Max for reporting the issue. I just took a quick look at the patch in comment #5 and have one suggestion that Mike previously mentioned. Make the name of the method sanitize_label() instead of just sanitize(). The reason is that a single method called sanitize within a file system class called fat16 implies that it somehow "sanitizes the fat16 file system". Since Mike has started the review process I'll leave it in his hands to perform the final review. Curtis
Created attachment 312587 [details] [review] Remove prohibited characters from FAT16/32 labels (v4) Hi Albert, I've modified your patch a bit. I started with version 2 of your patch from comment #4 and made some updates: 1) Renamed pad_label() to sanatize_label() as per my request. (In version 2 you put the removal of the prohibited characters in the requested place). 2) Stopped upper casing the label. [*] 3) Updated the removal of the prohibited chars code to simplify it by using find() so the inner loop can be removed. 4) Stopped using variables called *name* to avoid confusion as partition naming is something completely different to labelling a file system. 5) Enhanced the commit message so it fully explains what goes wrong and why, meaning in future we don't have to refer back to bugzilla to understand why this change was needed. Hi Curtis, Just want to draw your attention to this user visible change which is stripping prohibited characters from FAT file system labels. But I don't see any other easy solution to prevent GParted waiting forever. I'll be pushing this patch upstream unless I here otherwise. Thanks, Mike [*] Case of FAT16/32 file systems Microsoft document storing FAT labels in upper case [1]. You previously identified that Windows NT 4 stores the label in the provided case and that GParted shouldn't be upper casing them[2]. When GParted creates a FAT file system it uses mkdosfs/mkfs.fat from dosfstools which preserves the case of the label. But labelling a file system uses mlabel from mtools which upper cases it. Dosfslabel preserves the label when updating it but as been evaluated [3] and has several issues which which means GParted has to continue to use mlabel. [1] https://technet.microsoft.com/en-us/library/bb490925.aspx [2] Bug 700228 comment 4 - FAT16/32 labels are sometimes shows corrupted [3] Bug #576616 - Cannot set label of fat32 partition with dosfstools Handling of the case of FAT labels is completely inconsistent everywhere. Deciding to leave GParted as it is regarding this.
Hi Mike, If the mlabel command is already uppercasing FAT16/32 labels, then it should be okay to uppercase the label earlier. Hence I am in agreement with yourself and Albert regarding uppercasing the label in the sanitize_label() method. Albert, Since you provided the original patches, would you please take a look at the updated patch from Mike? Your name is on the patch so you should also be comfortable with the code. In terms of providing credit where credit is due, would you be okay if we included your name and email address in the AUTHORS file? https://git.gnome.org/browse/gparted/tree/AUTHORS Curtis
Created attachment 312607 [details] [review] Remove prohibited characters from FAT16/32 labels (v5) Hi All, Here's patch v5. Only difference with v4 is that it upper cases the FAT label when creating and labelling. I'll push this patch upstream unless I here otherwise. Thanks, Mike
(In reply to Curtis Gedak from comment #9) > Hi Mike, > > If the mlabel command is already uppercasing FAT16/32 labels, then it should > be okay to uppercase the label earlier. Hence I am in agreement with > yourself and Albert regarding uppercasing the label in the sanitize_label() > method. > > > Albert, > > Since you provided the original patches, would you please take a look at the > updated patch from Mike? > > Your name is on the patch so you should also be comfortable with the code. > > In terms of providing credit where credit is due, would you be okay if we > included your name and email address in the AUTHORS file? > > https://git.gnome.org/browse/gparted/tree/AUTHORS > > Curtis Hello Curtis, That sounds ok to me! This is my first contribution to a big, open source project, so I'm excited and surprised I was able to help. I looked at the code Mike submitted and it is more succinct than mine, so I have no objections. Thank you Mike for letting me work on this with you! Albert
Thank you Albert and Max, and welcome to the world of contributing to Free Software. :-) Thanks also Mike for overseeing this report and improving up on the initial patch. My testing of patch v5 in comment #10 went well and correctly prevented the hang situation raised in this report. Patch v5 has been committed to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Remove prohibited characters from FAT16/32 labels (#755608) https://git.gnome.org/browse/gparted/commit/?id=584137b32b4deed2c20022628baaee6b38570fa5 Provide credit for patch by Albert Young (#755608) https://git.gnome.org/browse/gparted/commit/?id=ee7d6dca203090566528e051e7f7361429614746
This enhancement was included in the GParted 0.24.0 release on October 27, 2015.