[bug report] fs: fat: create correct short names

Hello Heinrich Schuchardt,
The patch 28cef9ca2e86: "fs: fat: create correct short names" from Nov 20, 2020, leads to the following Smatch static checker warning:
fs/fat/fat_write.c:61 str2fat() warn: impossible condition '(c > 127) => ((-128)-127 > 127)'
fs/fat/fat_write.c:136 set_name() warn: impossible condition '(*dirent.name == 229) => ((-128)-127 == 229)'
fs/fat/fat_write.c 105 static int set_name(fat_itr *itr, const char *filename, char *shortname) 106 { 107 char *period; 108 char *pos; 109 int period_location; 110 char buf[13]; 111 int i; 112 int ret; 113 struct nameext dirent; 114 115 if (!filename) 116 return -EIO; 117 118 /* Initialize buffer */ 119 memset(&dirent, ' ', sizeof(dirent)); 120 121 /* Convert filename to upper case short name */ 122 period = strrchr(filename, '.'); 123 pos = (char *)filename; 124 if (*pos == '.') { 125 pos = period + 1; 126 period = 0; 127 } 128 if (period) 129 str2fat(dirent.ext, period + 1, sizeof(dirent.ext)); 130 period_location = str2fat(dirent.name, pos, sizeof(dirent.name)); 131 if (period_location < 0) 132 return period_location; 133 if (*dirent.name == ' ') 134 *dirent.name = '_'; 135 /* 0xe5 signals a deleted directory entry. Replace it by 0x05. */ --> 136 if (*dirent.name == 0xe5)
dirent.name is a char type. Chars are signed on probably most people's systems. The Linux kernel recently made char unsigned for everyone. Anyway in a char 0xe5 is negative but as a literal, it's positive so this condition cannot be true.
137 *dirent.name = 0x05; 138 139 /* If filename and short name are the same, quit. */ 140 sprintf(buf, "%.*s.%.3s", period_location, dirent.name, dirent.ext); 141 if (!strcmp(buf, filename)) { 142 ret = 1;
regards, dan carpenter

On 26.07.23 09:02, Dan Carpenter wrote:
Hello Heinrich Schuchardt,
The patch 28cef9ca2e86: "fs: fat: create correct short names" from Nov 20, 2020, leads to the following Smatch static checker warning:
fs/fat/fat_write.c:61 str2fat() warn: impossible condition '(c > 127) => ((-128)-127 > 127)' fs/fat/fat_write.c:136 set_name() warn: impossible condition '(*dirent.name == 229) => ((-128)-127 == 229)'
fs/fat/fat_write.c 105 static int set_name(fat_itr *itr, const char *filename, char *shortname) 106 { 107 char *period; 108 char *pos; 109 int period_location; 110 char buf[13]; 111 int i; 112 int ret; 113 struct nameext dirent; 114 115 if (!filename) 116 return -EIO; 117 118 /* Initialize buffer */ 119 memset(&dirent, ' ', sizeof(dirent)); 120 121 /* Convert filename to upper case short name */ 122 period = strrchr(filename, '.'); 123 pos = (char *)filename; 124 if (*pos == '.') { 125 pos = period + 1; 126 period = 0; 127 } 128 if (period) 129 str2fat(dirent.ext, period + 1, sizeof(dirent.ext)); 130 period_location = str2fat(dirent.name, pos, sizeof(dirent.name)); 131 if (period_location < 0) 132 return period_location; 133 if (*dirent.name == ' ') 134 *dirent.name = '_'; 135 /* 0xe5 signals a deleted directory entry. Replace it by 0x05. */ --> 136 if (*dirent.name == 0xe5)
dirent.name is a char type. Chars are signed on probably most people's systems. The Linux kernel recently made char unsigned for everyone. Anyway in a char 0xe5 is negative but as a literal, it's positive so this condition cannot be true.
137 *dirent.name = 0x05;
Thanks for reporting the issue. In the same file we have:
fs/fat/fat_write.c:1493: dent->nameext.name[0] = DELETED_FLAG;
include/fat.h:53: #define DELETED_FLAG ((char)0xe5)
I will send a patch.
Best regards
Heinrich
138 139 /* If filename and short name are the same, quit. */ 140 sprintf(buf, "%.*s.%.3s", period_location, dirent.name, dirent.ext); 141 if (!strcmp(buf, filename)) { 142 ret = 1;
regards, dan carpenter
participants (2)
-
Dan Carpenter
-
Heinrich Schuchardt