| Message ID | 1266903821-20800-1-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 2010-02-22, at 22:43, Theodore Ts'o wrote: > E2fsprogs 1.41.10 introduced a regression (in commit b71e018) where > e2fsck -fD can corrupt non-indexed directories when are exists one or > more file names which alphabetically sort before ".". This can happen > with ext2 filesystems or for small directories (take less than a > block) which contain filenames that begin with a space or some other > punctuation mark. > > Fix this by making sure we never reorder the '.' or '..' entry in the > directory, since they must be first. Ted, thanks for getting this out so fast. We were just building our release based on 1.41.10 and are able to add this in. Do you have a regression test for this case? e2fsck itself will detect the corruption ("." and ".." not at the start of the directory) after the fact, but I guess it means that there are no existing tests where there is a directory entry that sorts before "." or it would have been noticed earlier. Looking at the ASCII characters before "." it seems that CVS "old version" files with a leading ".#", "(", and "%gconf.xml" would probably be the most likely cause of problems. In any case, I can reproduce this easily on my filesystem with my mp3 collection (on a backup, fortunately). Unfortunately, it seems that re-running e2fsck after such a corruption causes all (thousands) of the entries that were sorted to the beginning of the directory to be deleted, and moved into /lost+found. Have you pulled this release from Sourceforge and any downstream releases already (Debian, FC, etc)? It seems like a pretty serious problem, even though "-fD" is likely not run very often. > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > e2fsck/rehash.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c > index 780742e..ceb8543 100644 > --- a/e2fsck/rehash.c > +++ b/e2fsck/rehash.c > @@ -763,7 +763,12 @@ retry_nohash: > > /* Sort the list */ > resort: > - qsort(fd.harray, fd.num_array, sizeof(struct hash_entry), hash_cmp); > + if (fd.compress) > + qsort(fd.harray+2, fd.num_array-2, sizeof(struct hash_entry), > + hash_cmp); > + else > + qsort(fd.harray, fd.num_array, sizeof(struct hash_entry), > + hash_cmp); > > /* > * Look for duplicates > -- > 1.6.6.1.1.g974db.dirty > > -- > To unsubscribe from this list: send the line "unsubscribe linux- > ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Canada, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 24, 2010 at 01:25:16AM -0700, Andreas Dilger wrote: > Do you have a regression test for this case? e2fsck itself will > detect the corruption ("." and ".." not at the start of the > directory) after the fact, but I guess it means that there are no > existing tests where there is a directory entry that sorts before > "." or it would have been noticed earlier. Yes, I'll add a regression test; binary files just don't work well in patch sets, so I tend to put those in separate commits, for ease in cherry picking. Basically it's just a 100k ext2 filesystem with a directory which happens to contain a name that begins with a open parenthesis, i.e., "(oops)". > Have you pulled this release from Sourceforge and any downstream > releases already (Debian, FC, etc)? It seems like a pretty serious > problem, even though "-fD" is likely not run very often. I was just going to accelerate getting 1.41.11 out the door, as opposed to going to the effort of trying to deprecate 1.41.10. In the case of Debian, and Ubuntu, it's too late already since 1.41.10 has already propagated out to bleeding-edge users. So the only way to pull it back would be to get a new release out the door, quickly... I guess I can easily enough pull it from kernel.org and make 1.41.9 the default release to download on sourceforge.net. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
tytso@mit.edu wrote: > On Wed, Feb 24, 2010 at 01:25:16AM -0700, Andreas Dilger wrote: >> Do you have a regression test for this case? e2fsck itself will >> detect the corruption ("." and ".." not at the start of the >> directory) after the fact, but I guess it means that there are no >> existing tests where there is a directory entry that sorts before >> "." or it would have been noticed earlier. > > Yes, I'll add a regression test; binary files just don't work well in > patch sets, so I tend to put those in separate commits, for ease in > cherry picking. Basically it's just a 100k ext2 filesystem with a > directory which happens to contain a name that begins with a open > parenthesis, i.e., "(oops)". > >> Have you pulled this release from Sourceforge and any downstream >> releases already (Debian, FC, etc)? It seems like a pretty serious >> problem, even though "-fD" is likely not run very often. > > I was just going to accelerate getting 1.41.11 out the door, as > opposed to going to the effort of trying to deprecate 1.41.10. In the > case of Debian, and Ubuntu, it's too late already since 1.41.10 has > already propagated out to bleeding-edge users. Ditto for fedora, but I pushed this patch to rawhide yesterday, thanks. -Eric > So the only way to > pull it back would be to get a new release out the door, quickly... > > I guess I can easily enough pull it from kernel.org and make > 1.41.9 the default release to download on sourceforge.net. > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/e2fsck/rehash.c b/e2fsck/rehash.c index 780742e..ceb8543 100644 --- a/e2fsck/rehash.c +++ b/e2fsck/rehash.c @@ -763,7 +763,12 @@ retry_nohash: /* Sort the list */ resort: - qsort(fd.harray, fd.num_array, sizeof(struct hash_entry), hash_cmp); + if (fd.compress) + qsort(fd.harray+2, fd.num_array-2, sizeof(struct hash_entry), + hash_cmp); + else + qsort(fd.harray, fd.num_array, sizeof(struct hash_entry), + hash_cmp); /* * Look for duplicates
E2fsprogs 1.41.10 introduced a regression (in commit b71e018) where e2fsck -fD can corrupt non-indexed directories when are exists one or more file names which alphabetically sort before ".". This can happen with ext2 filesystems or for small directories (take less than a block) which contain filenames that begin with a space or some other punctuation mark. Fix this by making sure we never reorder the '.' or '..' entry in the directory, since they must be first. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- e2fsck/rehash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)