| Message ID | 1373268366-14508-3-git-send-email-cngesaint@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 07/08 03:26, Xu Wang wrote: > Signed-off-by: Xu Wang <cngesaint@gmail.com> > --- > block.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 94 insertions(+) > > diff --git a/block.c b/block.c > index 53b1a01..8dc6ded 100644 > --- a/block.c > +++ b/block.c > @@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) > bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns; > } > > +#ifdef _WIN32 > +static int get_lnk_target_file(const char *lnk_file, char *filepath) > +{ > + unsigned int flag, offet; > + unsigned int sflag; > + char uch; > + int i = 0; > + > + FILE *fd = fopen(lnk_file, "rb"); > + if (!fd) { > + error_report("Open file %s failed.", lnk_file); > + return -1; > + } > + fread(&flag, 4, 1, fd); > + if (flag != 0x4c) { > + error_report("%s is not a lnk file.", lnk_file); > + fclose(fd); > + return -1; > + } > + fseek(fd, 0x14, SEEK_SET); > + fread(&flag, 4, 1, fd); > + fseek(fd, 0x4c, SEEK_SET); > + > + if (flag & 0x01) { > + fread(&sflag, 2, 1, fd); > + fseek(fd, sflag, SEEK_CUR); > + } > + > + offset = ftell(fd); > + fseek(fd, offset + 0x10, SEEK_SET); > + fread(&flag, 4, 1, fd); > + fseek(fd, flag + offset, SEEK_SET); > + > + do { > + fread(&uch, 1, 1, fd); > + filepath[i++] = uch; > + } while (uch != '\0'); Please limit the number of bytes to read to capacity of filepath, avoid the security hole. > + > + fclose(fd); > + return 0; > +} > + > +static long get_win_inode(const char *filename) > +{ > + char pbuf[MAX_PATH_LEN], *p; > + long inode; > + struct stat sbuf; > + char path[MAX_PATH_LEN]; > + > + /* If filename contains .lnk, it's a shortcuts. Target file > + * * need to be parsed. > + * */ > + if (strstr(filename, ".lnk")) { Please be more accurate with this checking, what if the filename happens to be "a.lnked.txt"? > + if (get_lnk_target_file(filename, path)) { > + error_report("Parse .lnk file %s failed.", filename); > + return -1; > + } > + } else { > + memcpy(path, filename, sizeof(filename)); > + } > + > + if (stat(path, &sbuf) == -1) { What's this stat() call for? > + error_report("get file %s stat error.", path); > + return -1; > + } > + if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) { How big is MAX_PATH_LEN? MSDN: If the buffer is too small to contain the path, the return value is the size, in TCHARs, of the buffer that is required to hold the path and the terminating null character. Please try to handle this case. (And is pbuf NULL terminated in this case?) > + inode = 11003; > + for (p = pbuf; *p != '\0'; p++) { > + inode = inode * 31 + *(unsigned char *)p; > + } > + return (inode * 911) & 0x7FFF; > + } > + > + return -1; > +} > +#endif > + > static gboolean str_equal_func(gconstpointer a, gconstpointer b) > { > return strcmp(a, b) == 0; > @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt, > error_report("Get file %s stat failed.", filename); > goto err; > } > + #ifdef _WIN32 > + inode = get_win_inode(filename); > + if (inode == -1) { > + error_report("Get file %s inode failed.", filename); > + goto err; > + } > + #else Move stat() call here? (seems not necessary for windows) > inode = (long)sbuf.st_ino; > + #endif > } > > filename = backing_file; > @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt, > error_report("Get file %s stat failed.", filename); > goto err; > } > + > + #ifdef _WIN32 > + inode = get_win_inode(filename); > + if (inode == -1) { > + error_report("Get file %s inode failed.", filename); > + goto err; > + } > + #else > inode = (long)sbuf.st_ino; See above. > + #endif > > if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) { > error_report("Backing file '%s' creates an infinite loop.", > -- > 1.8.1.4 > >
2013/7/10 Fam Zheng <famz@redhat.com> > On Mon, 07/08 03:26, Xu Wang wrote: > > Signed-off-by: Xu Wang <cngesaint@gmail.com> > > --- > > block.c | 94 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 94 insertions(+) > > > > diff --git a/block.c b/block.c > > index 53b1a01..8dc6ded 100644 > > --- a/block.c > > +++ b/block.c > > @@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs, > BlockAcctCookie *cookie) > > bs->total_time_ns[cookie->type] += get_clock() - > cookie->start_time_ns; > > } > > > > +#ifdef _WIN32 > > +static int get_lnk_target_file(const char *lnk_file, char *filepath) > > +{ > > + unsigned int flag, offet; > > + unsigned int sflag; > > + char uch; > > + int i = 0; > > + > > + FILE *fd = fopen(lnk_file, "rb"); > > + if (!fd) { > > + error_report("Open file %s failed.", lnk_file); > > + return -1; > > + } > > + fread(&flag, 4, 1, fd); > > + if (flag != 0x4c) { > > + error_report("%s is not a lnk file.", lnk_file); > > + fclose(fd); > > + return -1; > > + } > > + fseek(fd, 0x14, SEEK_SET); > > + fread(&flag, 4, 1, fd); > > + fseek(fd, 0x4c, SEEK_SET); > > + > > + if (flag & 0x01) { > > + fread(&sflag, 2, 1, fd); > > + fseek(fd, sflag, SEEK_CUR); > > + } > > + > > + offset = ftell(fd); > > + fseek(fd, offset + 0x10, SEEK_SET); > > + fread(&flag, 4, 1, fd); > > + fseek(fd, flag + offset, SEEK_SET); > > + > > + do { > > + fread(&uch, 1, 1, fd); > > + filepath[i++] = uch; > > + } while (uch != '\0'); > Please limit the number of bytes to read to capacity of filepath, avoid > the security hole. > @i was limited in [0, MAX_PATH_LEN] in the new version. > > + > > + fclose(fd); > > + return 0; > > +} > > + > > +static long get_win_inode(const char *filename) > > +{ > > + char pbuf[MAX_PATH_LEN], *p; > > + long inode; > > + struct stat sbuf; > > + char path[MAX_PATH_LEN]; > > + > > + /* If filename contains .lnk, it's a shortcuts. Target file > > + * * need to be parsed. > > + * */ > > + if (strstr(filename, ".lnk")) { > Please be more accurate with this checking, what if the filename happens > to be "a.lnked.txt"? > Generally speaking filename has no more than one extenstion name on Windows (I just found some virus could break that rule). Maybe I have not enough experience on WIN32 plateform, so I updated code will check whether the filename is end with '.lnk'. > > + if (get_lnk_target_file(filename, path)) { > > + error_report("Parse .lnk file %s failed.", filename); > > + return -1; > > + } > > + } else { > > + memcpy(path, filename, sizeof(filename)); > > + } > > + > > + if (stat(path, &sbuf) == -1) { > What's this stat() call for? > I read a lot code about get inode on the WIN32 plateform. All of them have stat calling. The function of stat() here is checking file by calling stat() if there was something wrong with this file, the following caculation is useless and return -1 directly. > > + error_report("get file %s stat error.", path); > > + return -1; > > + } > > + if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) { > How big is MAX_PATH_LEN? > MSDN: If the buffer is too small to contain the path, the return value > is the size, in TCHARs, of the buffer that is required to hold the path > and the terminating null character. Please try to handle this case. (And > is pbuf NULL terminated in this case?) > This is really a hard desicion to set value of it because length of path on Windows could be unlimited. So here I just set an value and want to get some tips from others. Now it's set as 8192 but I missed it when I made this patch. > > + inode = 11003; > > + for (p = pbuf; *p != '\0'; p++) { > > + inode = inode * 31 + *(unsigned char *)p; > > + } > > + return (inode * 911) & 0x7FFF; > > + } > > + > > + return -1; > > +} > > +#endif > > + > > static gboolean str_equal_func(gconstpointer a, gconstpointer b) > > { > > return strcmp(a, b) == 0; > > @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char > *filename, const char *fmt, > > error_report("Get file %s stat failed.", filename); > > goto err; > > } > > + #ifdef _WIN32 > > + inode = get_win_inode(filename); > > + if (inode == -1) { > > + error_report("Get file %s inode failed.", filename); > > + goto err; > > + } > > + #else > Move stat() call here? (seems not necessary for windows) > I decided to move all stat() into get_inode (I changed its name from get_win_inode()). Logic is more simpler now. Xu Wang > > inode = (long)sbuf.st_ino; > > + #endif > > } > > > > filename = backing_file; > > @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char > *filename, const char *fmt, > > error_report("Get file %s stat failed.", filename); > > goto err; > > } > > + > > + #ifdef _WIN32 > > + inode = get_win_inode(filename); > > + if (inode == -1) { > > + error_report("Get file %s inode failed.", filename); > > + goto err; > > + } > > + #else > > inode = (long)sbuf.st_ino; > See above. > > + #endif > > > > if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) { > > error_report("Backing file '%s' creates an infinite loop.", > > -- > > 1.8.1.4 > > > > > > -- > Fam >
On Mon, 07/15 14:09, Xu Wang wrote: > 2013/7/10 Fam Zheng <famz@redhat.com> > > > On Mon, 07/08 03:26, Xu Wang wrote: > > > + error_report("get file %s stat error.", path); > > > + return -1; > > > + } > > > + if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) { > > How big is MAX_PATH_LEN? > > MSDN: If the buffer is too small to contain the path, the return value > > is the size, in TCHARs, of the buffer that is required to hold the path > > and the terminating null character. Please try to handle this case. (And > > is pbuf NULL terminated in this case?) > > > This is really a hard desicion to set value of it because length of path on > Windows > could be unlimited. So here I just set an value and want to get some tips > from others. > Now it's set as 8192 but I missed it when I made this patch. > Yes, if you use heap allocated buffer, you can work with longer filename. If you use fixed length buffer, ideally you should detect whether return value is greater than your buffer size: in this case the pbuf content is invalid (or incomplete), so your hash computation below is inaccurate, consequentially false alarm.
diff --git a/block.c b/block.c index 53b1a01..8dc6ded 100644 --- a/block.c +++ b/block.c @@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie) bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns; } +#ifdef _WIN32 +static int get_lnk_target_file(const char *lnk_file, char *filepath) +{ + unsigned int flag, offet; + unsigned int sflag; + char uch; + int i = 0; + + FILE *fd = fopen(lnk_file, "rb"); + if (!fd) { + error_report("Open file %s failed.", lnk_file); + return -1; + } + fread(&flag, 4, 1, fd); + if (flag != 0x4c) { + error_report("%s is not a lnk file.", lnk_file); + fclose(fd); + return -1; + } + fseek(fd, 0x14, SEEK_SET); + fread(&flag, 4, 1, fd); + fseek(fd, 0x4c, SEEK_SET); + + if (flag & 0x01) { + fread(&sflag, 2, 1, fd); + fseek(fd, sflag, SEEK_CUR); + } + + offset = ftell(fd); + fseek(fd, offset + 0x10, SEEK_SET); + fread(&flag, 4, 1, fd); + fseek(fd, flag + offset, SEEK_SET); + + do { + fread(&uch, 1, 1, fd); + filepath[i++] = uch; + } while (uch != '\0'); + + fclose(fd); + return 0; +} + +static long get_win_inode(const char *filename) +{ + char pbuf[MAX_PATH_LEN], *p; + long inode; + struct stat sbuf; + char path[MAX_PATH_LEN]; + + /* If filename contains .lnk, it's a shortcuts. Target file + * * need to be parsed. + * */ + if (strstr(filename, ".lnk")) { + if (get_lnk_target_file(filename, path)) { + error_report("Parse .lnk file %s failed.", filename); + return -1; + } + } else { + memcpy(path, filename, sizeof(filename)); + } + + if (stat(path, &sbuf) == -1) { + error_report("get file %s stat error.", path); + return -1; + } + if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) { + inode = 11003; + for (p = pbuf; *p != '\0'; p++) { + inode = inode * 31 + *(unsigned char *)p; + } + return (inode * 911) & 0x7FFF; + } + + return -1; +} +#endif + static gboolean str_equal_func(gconstpointer a, gconstpointer b) { return strcmp(a, b) == 0; @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt, error_report("Get file %s stat failed.", filename); goto err; } + #ifdef _WIN32 + inode = get_win_inode(filename); + if (inode == -1) { + error_report("Get file %s inode failed.", filename); + goto err; + } + #else inode = (long)sbuf.st_ino; + #endif } filename = backing_file; @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt, error_report("Get file %s stat failed.", filename); goto err; } + + #ifdef _WIN32 + inode = get_win_inode(filename); + if (inode == -1) { + error_report("Get file %s inode failed.", filename); + goto err; + } + #else inode = (long)sbuf.st_ino; + #endif if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) { error_report("Backing file '%s' creates an infinite loop.",
Signed-off-by: Xu Wang <cngesaint@gmail.com> --- block.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+)