Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 12:56 PM Hans Petter Selasky wrote: > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. > No, that only applies when the length is omitted (char foo[] = "string" would have \0, but foo[sizeof("string") -1] = "string" won't). Cheers, > > --HPS > ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Sat, 2019-04-06 at 01:47 +1100, Bruce Evans wrote: > On Fri, 5 Apr 2019, Ed Maste wrote: > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > >> > > > >>> +static const u_char dot_name[] = { > >>> + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> +static const u_char dotdot_name[] = { > >>> + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> + > >> > >> Does it make since to encode these as hex or octal constants, > >> one can not tell that those are different values in an easy > >> manner. They all look like '.' in the diff, and probably > >> in most editors. > > No, but it makes sense to write them as string constants. They are just > the strings "." and ".." padded with spaces to length 11, except they > are not actually strings since they are not NUL terminated. 11 is for > 8+3 msdos short file names. These are not NUL terminated either, but > it should be easy to ignore the extra NUL given by the string constants. > Defining them as nulterminated strings will also affect sizeof(dot_name), better make sure nothing relies on that. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: > > > +static const u_char dot_name[] = { > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > +static const u_char dotdot_name[] = { > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > + > > Does it make since to encode these as hex or octal constants, > one can not tell that those are different values in an easy > manner. They all look like '.' in the diff, and probably > in most editors. They are all either '.' or ' ', the commas are just list separators. IMO spaces after the commas would make it slightly easier to see. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Apr 5, 2019, at 13:22, Rodney W. Grimes wrote: >>> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: >>> On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: >>> > +static const u_char dot_name[] = { > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > +static const u_char dotdot_name[] = { > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > + >>> >>> They are all either '.' or ' ', the commas are just list separators. >>> IMO spaces after the commas would make it slightly easier to see. >> >> Would it make sense to just use the normal string syntax for char >> array assignment? >> >> static const u_char dot_name[11] = ". "; >> static const u_char dotdot_name[11] = ".. "; >> >> Seems more clear to me. > > To try and end this thread, I already stated it was in review, > let me get a pointer to it, if you have comments please post > them there: > https://reviews.freebsd.org/D19829 It would probably be a good idea to add a comment in the code to document the intention. Thanks, -Enji ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 05, 2019 at 09:55:43PM +0200, Hans Petter Selasky wrote: > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. Only if the array length allows for it. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > > > > > > > > > +static const u_char dot_name[] = { > > > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > > +static const u_char dotdot_name[] = { > > > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > > + > > > > They are all either '.' or ' ', the commas are just list separators. > > IMO spaces after the commas would make it slightly easier to see. > > Would it make sense to just use the normal string syntax for char > array assignment? > > static const u_char dot_name[11] = ". "; > static const u_char dotdot_name[11] = ".. "; > > Seems more clear to me. To try and end this thread, I already stated it was in review, let me get a pointer to it, if you have comments please post them there: https://reviews.freebsd.org/D19829 -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Apr 5, 2019, at 13:22, Rodney W. Grimes wrote: > > >>> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > >>> > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > wrote: > > >>> > > +static const u_char dot_name[] = { > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > +static const u_char dotdot_name[] = { > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > + > >>> > >>> They are all either '.' or ' ', the commas are just list separators. > >>> IMO spaces after the commas would make it slightly easier to see. > >> > >> Would it make sense to just use the normal string syntax for char > >> array assignment? > >> > >> static const u_char dot_name[11] = ". "; > >> static const u_char dotdot_name[11] = ".. "; > >> > >> Seems more clear to me. > > > > To try and end this thread, I already stated it was in review, > > let me get a pointer to it, if you have comments please post > > them there: > > https://reviews.freebsd.org/D19829 > > It would probably be a good idea to add a comment in the code to document the > intention. > Thanks, Again, there is a review up, D19829, please comment there. > -Enji -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On 4/5/19 9:51 PM, Conrad Meyer wrote: static const u_char dot_name[11] = ". "; static const u_char dotdot_name[11] = ".. "; Seems more clear to me. Using this syntax will include a terminating zero. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > wrote: > > > > > > +static const u_char dot_name[] = { > > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > +static const u_char dotdot_name[] = { > > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > + > > They are all either '.' or ' ', the commas are just list separators. > IMO spaces after the commas would make it slightly easier to see. Would it make sense to just use the normal string syntax for char array assignment? static const u_char dot_name[11] = ". "; static const u_char dotdot_name[11] = ".. "; Seems more clear to me. Best, Conrad ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 12:56 PM Hans Petter Selasky wrote: > > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. Nope? (gdb) ptype foo type = const unsigned char [11] (gdb) p foo $3 = "abc" (gdb) p foo[10] $4 = 32 ' ' ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, 5 Apr 2019, Ed Maste wrote: On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: +static const u_char dot_name[] = { + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; +static const u_char dotdot_name[] = { + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; + Does it make since to encode these as hex or octal constants, one can not tell that those are different values in an easy manner. They all look like '.' in the diff, and probably in most editors. No, but it makes sense to write them as string constants. They are just the strings "." and ".." padded with spaces to length 11, except they are not actually strings since they are not NUL terminated. 11 is for 8+3 msdos short file names. These are not NUL terminated either, but it should be easy to ignore the extra NUL given by the string constants. They are all either '.' or ' ', the commas are just list separators. IMO spaces after the commas would make it slightly easier to see. The single quotes looking like commas indeed makes this hard to read. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Fri, 5 Apr 2019, Ed Maste wrote: > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > >> > > > >>> +static const u_char dot_name[] = { > >>> + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> +static const u_char dotdot_name[] = { > >>> + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> + > >> > >> Does it make since to encode these as hex or octal constants, > >> one can not tell that those are different values in an easy > >> manner. They all look like '.' in the diff, and probably > >> in most editors. > > No, but it makes sense to write them as string constants. They are just > the strings "." and ".." padded with spaces to length 11, except they > are not actually strings since they are not NUL terminated. 11 is for > 8+3 msdos short file names. These are not NUL terminated either, but > it should be easy to ignore the extra NUL given by the string constants. There is a review up, and that is exactly what has been done, dot_names[11] = ". " > > They are all either '.' or ' ', the commas are just list separators. > > IMO spaces after the commas would make it slightly easier to see. > > The single quotes looking like commas indeed makes this hard to read. Almost headache creating :-). > Bruce -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> Author: delphij > Date: Fri Apr 5 02:21:16 2019 > New Revision: 345900 > URL: https://svnweb.freebsd.org/changeset/base/345900 > > Log: > Implement checking of `.' and `..' entries of subdirectory. > > Reviewed by:pfg > Obtained from: Android > https://android.googlesource.com/platform/external/fsck_msdos/+/b6ee08aadb580341a4d80943741b80de16a88b5d%5E%21/ > MFC after: 2 weeks > Differential Revision: https://reviews.freebsd.org/D19824 > > Modified: > head/sbin/fsck_msdosfs/dir.c > > Modified: head/sbin/fsck_msdosfs/dir.c > == > --- head/sbin/fsck_msdosfs/dir.c Fri Apr 5 01:22:30 2019 > (r345899) > +++ head/sbin/fsck_msdosfs/dir.c Fri Apr 5 02:21:16 2019 > (r345900) > @@ -444,7 +444,78 @@ checksize(struct bootblock *boot, struct fatEntry *fat > return FSOK; > } > > +static const u_char dot_name[] = { > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > +static const u_char dotdot_name[] = { > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > + Does it make since to encode these as hex or octal constants, one can not tell that those are different values in an easy manner. They all look like '.' in the diff, and probably in most editors. > /* ... -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r345900 - head/sbin/fsck_msdosfs
Author: delphij Date: Fri Apr 5 02:21:16 2019 New Revision: 345900 URL: https://svnweb.freebsd.org/changeset/base/345900 Log: Implement checking of `.' and `..' entries of subdirectory. Reviewed by: pfg Obtained from:Android https://android.googlesource.com/platform/external/fsck_msdos/+/b6ee08aadb580341a4d80943741b80de16a88b5d%5E%21/ MFC after:2 weeks Differential Revision:https://reviews.freebsd.org/D19824 Modified: head/sbin/fsck_msdosfs/dir.c Modified: head/sbin/fsck_msdosfs/dir.c == --- head/sbin/fsck_msdosfs/dir.cFri Apr 5 01:22:30 2019 (r345899) +++ head/sbin/fsck_msdosfs/dir.cFri Apr 5 02:21:16 2019 (r345900) @@ -444,7 +444,78 @@ checksize(struct bootblock *boot, struct fatEntry *fat return FSOK; } +static const u_char dot_name[] = { + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; +static const u_char dotdot_name[] = { + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; + /* + * Basic sanity check if the subdirectory have good '.' and '..' entries, + * and they are directory entries. Further sanity checks are performed + * when we traverse into it. + */ +static int +check_subdirectory(int f, struct bootblock *boot, struct dosDirEntry *dir) +{ + u_char *buf, *cp; + off_t off; + cl_t cl; + int retval = FSOK; + + cl = dir->head; + if (dir->parent && (cl < CLUST_FIRST || cl >= boot->NumClusters)) { + return FSERROR; + } + + if (!(boot->flags & FAT32) && !dir->parent) { + off = boot->bpbResSectors + boot->bpbFATs * + boot->FATsecs; + } else { + off = cl * boot->bpbSecPerClust + boot->ClusterOffset; + } + + /* +* We only need to check the first two entries of the directory, +* which is found in the first sector of the directory entry, +* so read in only the first sector. +*/ + buf = malloc(boot->bpbBytesPerSec); + if (buf == NULL) { + perr("No space for directory buffer (%u)", + boot->bpbBytesPerSec); + return FSFATAL; + } + + off *= boot->bpbBytesPerSec; + if (lseek(f, off, SEEK_SET) != off || + read(f, buf, boot->bpbBytesPerSec) != boot->bpbBytesPerSec) { + perr("Unable to read directory"); + free(buf); + return FSFATAL; + } + + /* +* Both `.' and `..' must be present and be the first two entries +* and be ATTR_DIRECTORY of a valid subdirectory. +*/ + cp = buf; + if (memcmp(cp, dot_name, sizeof(dot_name)) != 0 || + (cp[11] & ATTR_DIRECTORY) != ATTR_DIRECTORY) { + pwarn("%s: Incorrect `.' for %s.\n", __func__, dir->name); + retval |= FSERROR; + } + cp += 32; + if (memcmp(cp, dotdot_name, sizeof(dotdot_name)) != 0 || + (cp[11] & ATTR_DIRECTORY) != ATTR_DIRECTORY) { + pwarn("%s: Incorrect `..' for %s. \n", __func__, dir->name); + retval |= FSERROR; + } + + free(buf); + return retval; +} + +/* * Read a directory and * - resolve long name records * - enter file and directory records into the parent's list @@ -491,9 +562,6 @@ readDosDirSection(int f, struct bootblock *boot, struc return FSFATAL; } last /= 32; - /* -* Check `.' and `..' entries here? XXX -*/ for (p = buffer, i = 0; i < last; i++, p += 32) { if (dir->fsckflags & DIREMPWARN) { *p = SLOT_EMPTY; @@ -830,6 +898,36 @@ readDosDirSection(int f, struct bootblock *boot, struc } } continue; + } else { + /* +* Only one directory entry can point +* to dir->head, it's '.'. +*/ + if (dirent.head == dir->head) { + pwarn("%s entry in %s has incorrect start cluster\n", + dirent.name, fullpath(dir)); + if (ask(1, "Remove")) { + *p = SLOT_DELETED; + mod |= THISMOD|FSDIRMOD; + } else + mod |= FSERROR; +
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 12:56 PM Hans Petter Selasky wrote: > > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. Nope? (gdb) ptype foo type = const unsigned char [11] (gdb) p foo $3 = "abc" (gdb) p foo[10] $4 = 32 ' ' ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Apr 5, 2019, at 13:22, Rodney W. Grimes wrote: > > >>> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > >>> > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > wrote: > > >>> > > +static const u_char dot_name[] = { > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > +static const u_char dotdot_name[] = { > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > + > >>> > >>> They are all either '.' or ' ', the commas are just list separators. > >>> IMO spaces after the commas would make it slightly easier to see. > >> > >> Would it make sense to just use the normal string syntax for char > >> array assignment? > >> > >> static const u_char dot_name[11] = ". "; > >> static const u_char dotdot_name[11] = ".. "; > >> > >> Seems more clear to me. > > > > To try and end this thread, I already stated it was in review, > > let me get a pointer to it, if you have comments please post > > them there: > > https://reviews.freebsd.org/D19829 > > It would probably be a good idea to add a comment in the code to document the > intention. > Thanks, Again, there is a review up, D19829, please comment there. > -Enji -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Apr 5, 2019, at 13:22, Rodney W. Grimes wrote: >>> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: >>> On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: >>> > +static const u_char dot_name[] = { > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > +static const u_char dotdot_name[] = { > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > + >>> >>> They are all either '.' or ' ', the commas are just list separators. >>> IMO spaces after the commas would make it slightly easier to see. >> >> Would it make sense to just use the normal string syntax for char >> array assignment? >> >> static const u_char dot_name[11] = ". "; >> static const u_char dotdot_name[11] = ".. "; >> >> Seems more clear to me. > > To try and end this thread, I already stated it was in review, > let me get a pointer to it, if you have comments please post > them there: > https://reviews.freebsd.org/D19829 It would probably be a good idea to add a comment in the code to document the intention. Thanks, -Enji ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 05, 2019 at 09:55:43PM +0200, Hans Petter Selasky wrote: > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. Only if the array length allows for it. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > > > > > > > > > +static const u_char dot_name[] = { > > > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > > +static const u_char dotdot_name[] = { > > > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > > + > > > > They are all either '.' or ' ', the commas are just list separators. > > IMO spaces after the commas would make it slightly easier to see. > > Would it make sense to just use the normal string syntax for char > array assignment? > > static const u_char dot_name[11] = ". "; > static const u_char dotdot_name[11] = ".. "; > > Seems more clear to me. To try and end this thread, I already stated it was in review, let me get a pointer to it, if you have comments please post them there: https://reviews.freebsd.org/D19829 -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 12:56 PM Hans Petter Selasky wrote: > On 4/5/19 9:51 PM, Conrad Meyer wrote: > > static const u_char dot_name[11] = ". "; > > static const u_char dotdot_name[11] = ".. "; > > > > Seems more clear to me. > > Using this syntax will include a terminating zero. > No, that only applies when the length is omitted (char foo[] = "string" would have \0, but foo[sizeof("string") -1] = "string" won't). Cheers, > > --HPS > ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On 4/5/19 9:51 PM, Conrad Meyer wrote: static const u_char dot_name[11] = ". "; static const u_char dotdot_name[11] = ".. "; Seems more clear to me. Using this syntax will include a terminating zero. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, Apr 5, 2019 at 6:49 AM Ed Maste wrote: > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > wrote: > > > > > > +static const u_char dot_name[] = { > > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > +static const u_char dotdot_name[] = { > > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > > + > > They are all either '.' or ' ', the commas are just list separators. > IMO spaces after the commas would make it slightly easier to see. Would it make sense to just use the normal string syntax for char array assignment? static const u_char dot_name[11] = ". "; static const u_char dotdot_name[11] = ".. "; Seems more clear to me. Best, Conrad ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Sat, 2019-04-06 at 01:47 +1100, Bruce Evans wrote: > On Fri, 5 Apr 2019, Ed Maste wrote: > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > >> > > > >>> +static const u_char dot_name[] = { > >>> + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> +static const u_char dotdot_name[] = { > >>> + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> + > >> > >> Does it make since to encode these as hex or octal constants, > >> one can not tell that those are different values in an easy > >> manner. They all look like '.' in the diff, and probably > >> in most editors. > > No, but it makes sense to write them as string constants. They are just > the strings "." and ".." padded with spaces to length 11, except they > are not actually strings since they are not NUL terminated. 11 is for > 8+3 msdos short file names. These are not NUL terminated either, but > it should be easy to ignore the extra NUL given by the string constants. > Defining them as nulterminated strings will also affect sizeof(dot_name), better make sure nothing relies on that. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> On Fri, 5 Apr 2019, Ed Maste wrote: > > > On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes > > wrote: > >> > > > >>> +static const u_char dot_name[] = { > >>> + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> +static const u_char dotdot_name[] = { > >>> + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > >>> + > >> > >> Does it make since to encode these as hex or octal constants, > >> one can not tell that those are different values in an easy > >> manner. They all look like '.' in the diff, and probably > >> in most editors. > > No, but it makes sense to write them as string constants. They are just > the strings "." and ".." padded with spaces to length 11, except they > are not actually strings since they are not NUL terminated. 11 is for > 8+3 msdos short file names. These are not NUL terminated either, but > it should be easy to ignore the extra NUL given by the string constants. There is a review up, and that is exactly what has been done, dot_names[11] = ". " > > They are all either '.' or ' ', the commas are just list separators. > > IMO spaces after the commas would make it slightly easier to see. > > The single quotes looking like commas indeed makes this hard to read. Almost headache creating :-). > Bruce -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, 5 Apr 2019, Ed Maste wrote: On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: +static const u_char dot_name[] = { + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; +static const u_char dotdot_name[] = { + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; + Does it make since to encode these as hex or octal constants, one can not tell that those are different values in an easy manner. They all look like '.' in the diff, and probably in most editors. No, but it makes sense to write them as string constants. They are just the strings "." and ".." padded with spaces to length 11, except they are not actually strings since they are not NUL terminated. 11 is for 8+3 msdos short file names. These are not NUL terminated either, but it should be easy to ignore the extra NUL given by the string constants. They are all either '.' or ' ', the commas are just list separators. IMO spaces after the commas would make it slightly easier to see. The single quotes looking like commas indeed makes this hard to read. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
On Fri, 5 Apr 2019 at 00:49, Rodney W. Grimes wrote: > > > +static const u_char dot_name[] = { > > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > +static const u_char dotdot_name[] = { > > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > > + > > Does it make since to encode these as hex or octal constants, > one can not tell that those are different values in an easy > manner. They all look like '.' in the diff, and probably > in most editors. They are all either '.' or ' ', the commas are just list separators. IMO spaces after the commas would make it slightly easier to see. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345900 - head/sbin/fsck_msdosfs
> Author: delphij > Date: Fri Apr 5 02:21:16 2019 > New Revision: 345900 > URL: https://svnweb.freebsd.org/changeset/base/345900 > > Log: > Implement checking of `.' and `..' entries of subdirectory. > > Reviewed by:pfg > Obtained from: Android > https://android.googlesource.com/platform/external/fsck_msdos/+/b6ee08aadb580341a4d80943741b80de16a88b5d%5E%21/ > MFC after: 2 weeks > Differential Revision: https://reviews.freebsd.org/D19824 > > Modified: > head/sbin/fsck_msdosfs/dir.c > > Modified: head/sbin/fsck_msdosfs/dir.c > == > --- head/sbin/fsck_msdosfs/dir.c Fri Apr 5 01:22:30 2019 > (r345899) > +++ head/sbin/fsck_msdosfs/dir.c Fri Apr 5 02:21:16 2019 > (r345900) > @@ -444,7 +444,78 @@ checksize(struct bootblock *boot, struct fatEntry *fat > return FSOK; > } > > +static const u_char dot_name[] = { > + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > +static const u_char dotdot_name[] = { > + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; > + Does it make since to encode these as hex or octal constants, one can not tell that those are different values in an easy manner. They all look like '.' in the diff, and probably in most editors. > /* ... -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r345900 - head/sbin/fsck_msdosfs
Author: delphij Date: Fri Apr 5 02:21:16 2019 New Revision: 345900 URL: https://svnweb.freebsd.org/changeset/base/345900 Log: Implement checking of `.' and `..' entries of subdirectory. Reviewed by: pfg Obtained from:Android https://android.googlesource.com/platform/external/fsck_msdos/+/b6ee08aadb580341a4d80943741b80de16a88b5d%5E%21/ MFC after:2 weeks Differential Revision:https://reviews.freebsd.org/D19824 Modified: head/sbin/fsck_msdosfs/dir.c Modified: head/sbin/fsck_msdosfs/dir.c == --- head/sbin/fsck_msdosfs/dir.cFri Apr 5 01:22:30 2019 (r345899) +++ head/sbin/fsck_msdosfs/dir.cFri Apr 5 02:21:16 2019 (r345900) @@ -444,7 +444,78 @@ checksize(struct bootblock *boot, struct fatEntry *fat return FSOK; } +static const u_char dot_name[] = { + '.', ' ',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; +static const u_char dotdot_name[] = { + '.', '.',' ',' ',' ',' ',' ',' ',' ',' ',' ' }; + /* + * Basic sanity check if the subdirectory have good '.' and '..' entries, + * and they are directory entries. Further sanity checks are performed + * when we traverse into it. + */ +static int +check_subdirectory(int f, struct bootblock *boot, struct dosDirEntry *dir) +{ + u_char *buf, *cp; + off_t off; + cl_t cl; + int retval = FSOK; + + cl = dir->head; + if (dir->parent && (cl < CLUST_FIRST || cl >= boot->NumClusters)) { + return FSERROR; + } + + if (!(boot->flags & FAT32) && !dir->parent) { + off = boot->bpbResSectors + boot->bpbFATs * + boot->FATsecs; + } else { + off = cl * boot->bpbSecPerClust + boot->ClusterOffset; + } + + /* +* We only need to check the first two entries of the directory, +* which is found in the first sector of the directory entry, +* so read in only the first sector. +*/ + buf = malloc(boot->bpbBytesPerSec); + if (buf == NULL) { + perr("No space for directory buffer (%u)", + boot->bpbBytesPerSec); + return FSFATAL; + } + + off *= boot->bpbBytesPerSec; + if (lseek(f, off, SEEK_SET) != off || + read(f, buf, boot->bpbBytesPerSec) != boot->bpbBytesPerSec) { + perr("Unable to read directory"); + free(buf); + return FSFATAL; + } + + /* +* Both `.' and `..' must be present and be the first two entries +* and be ATTR_DIRECTORY of a valid subdirectory. +*/ + cp = buf; + if (memcmp(cp, dot_name, sizeof(dot_name)) != 0 || + (cp[11] & ATTR_DIRECTORY) != ATTR_DIRECTORY) { + pwarn("%s: Incorrect `.' for %s.\n", __func__, dir->name); + retval |= FSERROR; + } + cp += 32; + if (memcmp(cp, dotdot_name, sizeof(dotdot_name)) != 0 || + (cp[11] & ATTR_DIRECTORY) != ATTR_DIRECTORY) { + pwarn("%s: Incorrect `..' for %s. \n", __func__, dir->name); + retval |= FSERROR; + } + + free(buf); + return retval; +} + +/* * Read a directory and * - resolve long name records * - enter file and directory records into the parent's list @@ -491,9 +562,6 @@ readDosDirSection(int f, struct bootblock *boot, struc return FSFATAL; } last /= 32; - /* -* Check `.' and `..' entries here? XXX -*/ for (p = buffer, i = 0; i < last; i++, p += 32) { if (dir->fsckflags & DIREMPWARN) { *p = SLOT_EMPTY; @@ -830,6 +898,36 @@ readDosDirSection(int f, struct bootblock *boot, struc } } continue; + } else { + /* +* Only one directory entry can point +* to dir->head, it's '.'. +*/ + if (dirent.head == dir->head) { + pwarn("%s entry in %s has incorrect start cluster\n", + dirent.name, fullpath(dir)); + if (ask(1, "Remove")) { + *p = SLOT_DELETED; + mod |= THISMOD|FSDIRMOD; + } else + mod |= FSERROR; +