[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 09:39:49 +0100 (CET)

> On February 5, 2014 at 2:49 AM jscipione@xxxxxxxxx wrote:
> c79381d: exfat: limit backup volume name to 11 characters

This is quite messy...

> +++ b/src/add-ons/kernel/file_systems/exfat/Volume.cpp
> @@ -377,8 +377,8 @@ Volume::Mount(const char* deviceName, uint32 flags)
>               double size = double((10 * diskSize + divisor - 1) / divisor);
>                       // %g in the kernel does not support precision...
> 
> -             snprintf(fName, sizeof(fName), "%g %cB ExFAT Volume",
> -                     size / 10, unit);
> +             snprintf(fName, 11 + 1, "%g %cB ExFAT", size / 10, unit);
> +                     // exfat volume names can only contain 11 characters

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.

> +++ b/src/add-ons/kernel/file_systems/exfat/Utility.cpp
[...]
> + *           Axel Dörfler, axeld@xxxxxxxxxxxxxxxx

I wasn't involved here AFAICT.

> +status_t
> +volume_name(struct exfat_entry* entry, char* _name)
> +{
> +     if (entry == NULL || _name == NULL)
> +             return B_BAD_VALUE;
> +
> +     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?)

> +             size_t length = strlen(untitled);
> +             strncpy(_name, untitled, length);

Why use strncpy()? It doesn't make any sense.

> +             if (strlen(_name) < length)
> +                     return B_NAME_TOO_LONG;

How should that happen?

> +     } else if (entry->type == EXFAT_ENTRY_TYPE_LABEL) {
> +             // UCS-2 can encode codepoints in the range U+0000 to U+FFFF
> +             // UTF-8 needs at most 3 bytes to encode values in this range
> +             size_t utf8NameLength = entry->label.length * 3;

volume_name() should get the buffer length; the name that is returned is pretty
much independent from exFAT (like "Untitled").
Also, we don't prefix buffers to be filled, only variables that are overwritten.
Ie. only for char** _name.
Also, the name should be get_volume_name(), as it doesn't return the name, but a
status.

> +             memset(_name, 0, utf8NameLength + 1);
> +                     // zero out the character array

Why? This doesn't seem to happen at other invocations of unicode_to_utf8().

> +/*!  Reads the volume name from an exfat entry and writes it to \a _name
> +     as a UTF-8 char array. Writes "Untitled" The volume name is not set.

"The" -> "if the".

> -     fVolume->SetName(utfName);
> +     char name[34];

Isn't there some constant you can use instead?

> -                     char                            fName[32];
> +                     char                            fName[34];
> +                             // Max number of bytes needed is (11 * 3) + 1
> for the \0,
> +                             // that is 11 UCS-2 characters converted to
> UTF-8.

Same here. Well, just introduce one, if it's not there already.

>  struct identify_cookie {
>       exfat_super_block super_block;
> +     char name[34];

And here as well.

>       memcpy(&cookie->super_block, &superBlock, sizeof(exfat_super_block));
> +     memset(cookie->name, 0, 34);
> +             // zero out volume name

Ouch. Ever heard of sizeof()?

> -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.

Bye,
   Axel.

Other related posts: