[haiku-commits] Re: haiku: hrev46820 - src/add-ons/kernel/file_systems/exfat

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Wed, 5 Feb 2014 12:24:20 +0100 (CET)

> On February 5, 2014 at 11:29 AM John Scipione <jscipione@xxxxxxxxx> wrote:
> > This is just wrong, please revert. fName is 34 bytes long, and snprintf()
> > operates on bytes.
> > Please just leave the sizeof(fName) in there; there is no reason to
> > cripple the name to 11 bytes here.
> I may need the result to fit back into 11 utf-16 characters to store it
> back into the volume label.

No, you don't. The volume size label is intended to make it possible to
distinguish unlabeled volumes better, because other platforms usually either
don't use volume labels, or at least don't force you to use them.
Haiku supports longer volume labels, and will also try to set them in case you
edit them. It's up to exFAT what it does with those longer names.

> It's a limitation of exfat.

Again, this is not meant to be written into exFAT structures.

> > > +     if (entry->type == EXFAT_ENTRY_TYPE_NOT_IN_USE) {
> > > +             const char* untitled = "Untitled";
> > Isn't that where the size of the volume should be used instead? (ie. the
> > code above?)
> Maybe... You are allowed to have a labeless volume, it's up to us to decide
> what to do in this case. The fallback name is only used for the volume
> name, not the mount point currently but I suppose I could change that.

The mount point is less important, the volume label is what is important,
though, as that's visible in the UI.
Of course, having the two of them as consistent as possible would be nice.

In general, the upper layers should probably be aware that this file system does
not have a label, and create the mount point name, as well as the volume name
itself (based on size, and file system name). But that's not in the scope of
this patch, of course.

> > > +             size_t length = strlen(untitled);
> > > +             strncpy(_name, untitled, length);
> > Why use strncpy()? It doesn't make any sense.
> If you pass a buffer too small I could buffer overrun with strcpy here and
> crash so I'm being a bit cautious.

Pardon my bluntness (as Koki would have put it), but you're not cautious, you
seem to be dumb:
volume_name() does not know anything about the size of the target buffer. How
can strncpy() change this, I wonder?
The only strncpy() does is stop copying if "length" is larger than the length of
the source buffer. Do you see what you are doing here?
If not, please stay away a few miles from kernel code.
Also note that strncpy() does not generate null terminated strings. Just never
use it, and there is one more chance to not make a mistake.

> > +             if (strlen(_name) < length)
> > > +                     return B_NAME_TOO_LONG;
> > How should that happen?
> Just being cautious checking to make sure the buffer you passed in
> was large enough to fit "Untitled"

Please see above.

> > volume_name() should get the buffer length; the name that is returned is
> > pretty much independent from exFAT (like "Untitled").
> The way I wrote it you are responsible for providing a buffer that's large
> enough or you'll get B_NAME_TOO_LONG. The calculation is for the conversion
> from Unicode to UTF8. I suppose I could have you pass in a buffer size.

No, the way you wrote it is simply broken.

> > > +             memset(_name, 0, utf8NameLength + 1);
> > > +                     // zero out the character array
> > Why? This doesn't seem to happen at other invocations of unicode_to_utf8().
> Perhaps this can be omitted, I'll have to doublecheck.

If not, the rest of the uses of that function are broken.

> > >       memcpy(&cookie->super_block, &superBlock,
> > sizeof(exfat_super_block));
> > > +     memset(cookie->name, 0, 34);
> > > +             // zero out volume name
> > Ouch. Ever heard of sizeof()?
> What's wrong with 34? That's the size, I know it is since I set it a few
> lines up. With a magic constant this might read better but I think it's
> valid to specify a constant size here.

Again, use sizeof(). If you change the size of cookie::name, this code will be
broken.
You don't want to have to maintain 57 places after you make such a change. You
only want to change a single place, and have the rest of the code either not
compile or handle the change as it should.

> > -exfat_scan_partition(int fd, partition_data *partition, void *_cookie)
> > > +exfat_scan_partition(int fd, partition_data* _partition, void* _cookie)
> > Again, do not prefix buffers or structures that are filled.
> Are you sure about that? The only reason I changed it is because I was
> confused about whether partition was suppose to be filled out already or
> not. The underscore helps me to know that I am expected to fill it with
> data here.

Yes, I am sure. After all, I invented that rule: I used to use the '_' prefix to
mark indirect variable access. This just indicates that the *pointer* will be
overwritten. I'm not even sure that this is actually part of our coding style
guide.

In this case, partition_data is even filled upfront, you just have to fill in
the missing pieces.

Bye,
   Axel.

Other related posts: