vvfat: some fixes
cover vvfat: some fixes
Hi,
This patchset is a follow-up for patch series sent here:
http://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05017.html
Patches 1 and 2 define and use some constants to make the code more clear. Patch 3 make read-write mode work when using non-ASCII filenames. Patch 4 fixes the last problem reported by MS Scandisk.
Hervé
Hervé Poussineau (4):
vvfat: add constants for special values of name[0]
vvfat: add a constant for bootsector name
vvfat: correctly parse non-ASCII short and long file names
vvfat: initialize memory after allocating it
block/vvfat.c | 85 +++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 29 deletions(-)
Signature · 1 line
2.11.0
PATCH 1/4 vvfat: add constants for special values of name[0]
--- block/vvfat.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
block/vvfat.c +11 -6
@@ -71,6 +71,11 @@ void nonono(const char* file, int line, const char* msg) {#endif +#define DIR_DELETED 0xe5 +#define DIR_KANJI DIR_DELETED +#define DIR_KANJI_FAKE 0x05 +#define DIR_FREE 0x00 +/* dynamic array functions */ typedef struct array_t { char* pointer;
@@ -466,7 +471,7 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)static char is_free (const direntry_t * direntry) { -return direntry->name[0 ]==0xe5 || direntry->name[0 ]==0x00 ; +return direntry->name[0 ] ==DIR_DELETED || direntry->name[0 ] ==DIR_FREE ; }static char is_volume_label (const direntry_t * direntry)
@@ -487,7 +492,7 @@ static char is_short_name(const direntry_t* direntry)static char is_directory (const direntry_t * direntry) { -return direntry->attributes &0x10 && direntry->name[0 ] !=0xe5 ; +return direntry->attributes &0x10 && direntry->name[0 ] !=DIR_DELETED ; }static inline char is_dot (const direntry_t * direntry)
@@ -589,8 +594,8 @@ static direntry_t *create_short_filename(BDRVVVFATState *s, } } -if (entry->name[0 ] ==0xe5 ) { - entry->name[0 ] =0x05 ; +if (entry->name[0 ] ==DIR_KANJI ) { + entry->name[0 ] =DIR_KANJI_FAKE ; }/* numeric-tail generation */
@@ -1748,8 +1753,8 @@ static int parse_short_name(BDRVVVFATState* s, }else lfn->name[i + j +1 ] ='\0' ; -if (lfn->name[0 ] ==0x05 ) { - lfn->name[0 ] =0xe5 ; +if (lfn->name[0 ] ==DIR_KANJI_FAKE ) { + lfn->name[0 ] =DIR_KANJI ; } lfn->len =strlen ((char *)lfn->name);
Signature · 1 line
2.11.0
PATCH 2/4 vvfat: add a constant for bootsector name
In reply to Hervé PoussineauAlso add links to related compatibility problems.
--- block/vvfat.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
block/vvfat.c +7 -1
@@ -71,6 +71,12 @@ void nonono(const char* file, int line, const char* msg) {#endif +/* bootsector OEM name. see related compatibility problems at: +* https://jdebp.eu/FGA/volume-boot-block-oem-name-field.html +* http://seasip.info/Misc/oemid.html +*/ +#define BOOTSECTOR_OEM_NAME "MSWIN4.1" +#define DIR_DELETED 0xe5#define DIR_KANJI DIR_DELETED#define DIR_KANJI_FAKE 0x05
@@ -1028,7 +1034,7 @@ static int init_directories(BDRVVVFATState* s, bootsector->jump[0 ]=0xeb ; bootsector->jump[1 ]=0x3e ; bootsector->jump[2 ]=0x90 ; -memcpy (bootsector->name,"MSWIN4.1" ,8 ); +memcpy (bootsector->name,BOOTSECTOR_OEM_NAME ,8 ); bootsector->sector_size=cpu_to_le16 (0x200 ); bootsector->sectors_per_cluster=s->sectors_per_cluster; bootsector->reserved_sectors=cpu_to_le16 (1 );
Signature · 1 line
2.11.0
Re: [Qemu-devel] [PATCH 2/4] vvfat: add a constant for bootsector name
Quoted 3 lines · Hervé Poussineau
Also add links to related compatibility problems.
Signed-off-by Hervé Poussineau <[email protected]>
Quoted 31 lines
--- block/vvfat.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)block/vvfat.c +7
-1@@ -71,6 +71,12 @@ void nonono(const char* file, int line, const char* msg) {#endif +/* bootsector OEM name. see related compatibility problems at: +* https://jdebp.eu/FGA/volume-boot-block-oem-name-field.html +* http://seasip.info/Misc/oemid.html +*/ +#define BOOTSECTOR_OEM_NAME "MSWIN4.1" +#define DIR_DELETED 0xe5#define DIR_KANJI DIR_DELETED#define DIR_KANJI_FAKE 0x05@@ -1028,7 +1034,7 @@ static int init_directories(BDRVVVFATState* s, bootsector->jump[0 ]=0xeb ; bootsector->jump[1 ]=0x3e ; bootsector->jump[2 ]=0x90 ; -memcpy (bootsector->name,"MSWIN4.1" ,8 ); +memcpy (bootsector->name,BOOTSECTOR_OEM_NAME ,8 ); bootsector->sector_size=cpu_to_le16 (0x200 ); bootsector->sectors_per_cluster=s->sectors_per_cluster; bootsector->reserved_sectors=cpu_to_le16 (1 );
PATCH 3/4 vvfat: correctly parse non-ASCII short and long file names
In reply to Hervé PoussineauWrite support works again when image contains non-ASCII names. It is either the case when user created a non-ASCII filename, or when initial directory contained a non-ASCII filename (since 0c36111f57ec2188f679e7fa810291b7386bdca1)
--- block/vvfat.c | 59 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 22 deletions(-)
block/vvfat.c +37 -22
@@ -1669,6 +1669,7 @@ typedef struct {* filename length is 0x3f * 13 bytes. */ unsigned char name[0x3f *13 +1 ]; +gunichar2 name2[0x3f *13 +1 ];int checksum, len;int sequence_number; } long_file_name;
@@ -1690,16 +1691,21 @@ static int parse_long_name(long_file_name* lfn,return 1 ;if (pointer[0 ] &0x40 ) { +/* first entry; do some initialization */ lfn->sequence_number = pointer[0 ] &0x3f ; lfn->checksum = pointer[13 ]; lfn->name[0 ] =0 ; lfn->name[lfn->sequence_number *13 ] =0 ; - }else if ((pointer[0 ] &0x3f ) != --lfn->sequence_number) + }else if ((pointer[0 ] &0x3f ) != --lfn->sequence_number) { +/* not the expected sequence number */ return -1 ; -else if (pointer[13 ] != lfn->checksum) + }else if (pointer[13 ] != lfn->checksum) { +/* not the expected checksum */ return -2 ; -else if (pointer[12 ] || pointer[26 ] || pointer[27 ]) + }else if (pointer[12 ] || pointer[26 ] || pointer[27 ]) { +/* invalid zero fields */ return -3 ; + } offset =13 * (lfn->sequence_number -1 );for (i =0 , j =1 ; i <13 ; i++, j+=2 ) {
@@ -1708,16 +1714,29 @@ static int parse_long_name(long_file_name* lfn,else if (j ==26 ) j =28 ; -if (pointer[j+1 ] ==0 ) - lfn->name[offset + i] = pointer[j]; -else if (pointer[j+1 ] !=0xff || (pointer[0 ] &0x40 ) ==0 ) -return -4 ; -else - lfn->name[offset + i] =0 ; +if (pointer[j] ==0 && pointer[j +1 ] ==0 ) { +/* end of long file name */ +break ; + } +gunichar2 c = (pointer[j +1 ] <<8 ) + pointer[j]; + lfn->name2[offset + i] = c; } -if (pointer[0 ] &0x40 ) - lfn->len = offset +strlen ((char *)lfn->name + offset); +if (pointer[0 ] &0x40 ) { +/* first entry; set len */ + lfn->len = offset + i; + } +if ((pointer[0 ] &0x3f ) ==0x01 ) { +/* last entry; finalize entry */ +glong olen; +gchar *utf8 =g_utf16_to_utf8 (lfn->name2, lfn->len,NULL , &olen,NULL ); +if (!utf8) { +return -4 ; + } + lfn->len = olen; +memcpy (lfn->name, utf8, olen +1 ); +g_free (utf8); + }return 0 ; }
@@ -1733,12 +1752,14 @@ static int parse_short_name(BDRVVVFATState* s,for (j =7 ; j >=0 && direntry->name[j] ==' ' ; j--);for (i =0 ; i <= j; i++) { -if (direntry->name[i] <=' ' || direntry->name[i] >0x7f ) +uint8_t c = direntry->name[i]; +if (c !=to_valid_short_char (c)) {return -1 ; -else if (s->downcase_short_names) + }else if (s->downcase_short_names) { lfn->name[i] =qemu_tolower (direntry->name[i]); -else + }else { lfn->name[i] = direntry->name[i]; + } }for (j =2 ; j >=0 && direntry->name[8 + j] ==' ' ; j--) {
@@ -1748,7 +1769,7 @@ static int parse_short_name(BDRVVVFATState* s, lfn->name[i + j +1 ] ='\0' ;for (;j >=0 ; j--) {uint8_t c = direntry->name[8 + j]; -if (c <=' ' || c >0x7f ) { +if (c !=to_valid_short_char (c)) {return -2 ; }else if (s->downcase_short_names) { lfn->name[i + j] =qemu_tolower (c);
@@ -2966,7 +2987,6 @@ DLOG(checkpoint());/* * Some sanity checks: * - do not allow writing to the boot sector -* - do not allow to write non-ASCII filenames */ if (sector_num < s->offset_to_fat)
@@ -3000,13 +3020,8 @@ DLOG(checkpoint()); direntries = (direntry_t *)(buf +0x200 * (begin - sector_num));for (k =0 ; k < (end - begin) *0x10 ; k++) { -/* do not allow non-ASCII filenames */ -if (parse_long_name (&lfn, direntries + k) <0 ) { -fprintf (stderr,"Warning: non-ASCII filename\n" ); -return -1 ; - }/* no access to the direntry of a read-only file */ -else if (is_short_name (direntries+k) && +if (is_short_name (direntries + k) && (direntries[k].attributes &1 )) { if (memcmp(direntries + k, array_get(&(s->directory), dir_index + k),
Signature · 1 line
2.11.0
PATCH 4/4 vvfat: initialize memory after allocating it
In reply to Hervé PoussineauThis prevents some host to guest memory content leaks.
--- block/vvfat.c | 1 + 1 file changed, 1 insertion(+)
block/vvfat.c +1 -0
@@ -115,6 +115,7 @@ static inline int array_ensure_allocated(array_t* array, int index) array->pointer =g_realloc (array->pointer, new_size);if (!array->pointer)return -1 ; +memset (array->pointer + array->size,0 , new_size - array->size); array->size = new_size; array->next = index +1 ; }
Signature · 1 line
2.11.0
Re: [Qemu-devel] [PATCH 4/4] vvfat: initialize memory after allocating it
Hi Hervé,
Quoted 14 lines · Hervé Poussineau
This prevents some host to guest memory content leaks.
Signed-off-by Hervé Poussineau <[email protected]>--- block/vvfat.c | 1 + 1 file changed, 1 insertion(+)block/vvfat.c +0
-0
block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 117
Quoted 3 lines
block/vvfat.c +0
-0@@ -115,6 +115,7 @@ static inline int array_ensure_allocated(array_t* array, int index) array->pointer =g_realloc (array->pointer, new_size);if (!array->pointer)return -1 ;
isn't it safer:
if (likely(new_size > array->size)) {
block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 118
Quoted 1 line
block/vvfat.c +1
-0+memset (array->pointer + array->size,0 , new_size - array->size);
}
?
block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 121
Quoted 4 lines
block/vvfat.c +0
-0array->size = new_size; array->next = index +1 ; }
Regards,
Phil.
Re: [Qemu-devel] [PATCH 4/4] vvfat: initialize memory after allocating it
Quoted 23 lines · Philippe Mathieu-Daudé
Hi Hervé,
Quoted 14 lines · Hervé Poussineau
This prevents some host to guest memory content leaks.
Signed-off-by Hervé Poussineau <[email protected]>--- block/vvfat.c | 1 + 1 file changed, 1 insertion(+)block/vvfat.c +0
-0block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 117
Quoted 3 lines
block/vvfat.c +0
-0@@ -115,6 +115,7 @@ static inline int array_ensure_allocated(array_t* array, int index) array->pointer =g_realloc (array->pointer, new_size);if (!array->pointer)return -1 ;isn't it safer:
Quoted 1 line
if (
likely(new_size > array->size)) {
Not really, because the code is:
if((index + 1) * array->item_size > array->size) {
int new_size = (index + 32) * array->item_size;
array->pointer = g_realloc(array->pointer, new_size);
if (!array->pointer)
return -1;
array->size = new_size;
array->next = index + 1;
}array->size is the size (in bytes) of the previous array. new_size is (index + 32) * item_size
And, due to the "if", we know that (index + 1) * item_size > array->size. So, new_size > array->size.
Quoted 16 lines
block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 118
Quoted 2 lines
block/vvfat.c +1
-0+memset (array->pointer + array->size,0 , new_size - array->size);}
?
block/vvfat.c static inline int array_ensure_allocated(array_t* array, int index) · line 121
Quoted 4 lines
block/vvfat.c +0
-0array->size = new_size; array->next = index +1 ; }Regards,
Phil.
Kevin Wolf
AppliedAm 15.07.2017 um 15:28 hat Hervé Poussineau geschrieben:
Quoted 10 lines
Hi,
This patchset is a follow-up for patch series sent here:
http://lists.gnu.org/archive/html/qemu-devel/2017-05/msg05017.htmlPatches 1 and 2 define and use some constants to make the code more clear. Patch 3 make read-write mode work when using non-ASCII filenames. Patch 4 fixes the last problem reported by MS Scandisk.
Hervé
Thanks, applied to the block branch.
Kevin