From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uybz0-0004JE-Ge for qemu-devel@nongnu.org; Mon, 15 Jul 2013 02:10:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uybyx-0006U1-5Q for qemu-devel@nongnu.org; Mon, 15 Jul 2013 02:10:02 -0400 Received: from mail-pd0-x230.google.com ([2607:f8b0:400e:c02::230]:46160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uybyw-0006Tt-Pt for qemu-devel@nongnu.org; Mon, 15 Jul 2013 02:09:59 -0400 Received: by mail-pd0-f176.google.com with SMTP id t12so10371838pdi.21 for ; Sun, 14 Jul 2013 23:09:57 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20130710104455.GB18465@T430s.nay.redhat.com> References: <1373268366-14508-1-git-send-email-cngesaint@gmail.com> <1373268366-14508-3-git-send-email-cngesaint@gmail.com> <20130710104455.GB18465@T430s.nay.redhat.com> Date: Mon, 15 Jul 2013 14:09:57 +0800 Message-ID: From: Xu Wang Content-Type: multipart/alternative; boundary=bcaec51dd56db3edce04e186b5ec Subject: Re: [Qemu-devel] [PATCH V2 2/5] Add WIN32 platform support for backing_file_loop_check() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: famz@redhat.com Cc: kwolf@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, xiawenc@linux.vnet.ibm.com --bcaec51dd56db3edce04e186b5ec Content-Type: text/plain; charset=ISO-8859-1 2013/7/10 Fam Zheng > On Mon, 07/08 03:26, Xu Wang wrote: > > Signed-off-by: Xu Wang > > --- > > 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 > --bcaec51dd56db3edce04e186b5ec Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable



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>
> ---
> =A0block.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++= +++++++++++
> =A01 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, BlockAcctC= ookie *cookie)
> =A0 =A0 =A0bs->total_time_ns[cookie->type] +=3D get_clock() - co= okie->start_time_ns;
> =A0}
>
> +#ifdef _WIN32
> +static int get_lnk_target_file(const char *lnk_file, char *filepath)<= br> > +{
> + =A0 =A0unsigned int flag, offet;
> + =A0 =A0unsigned int sflag;
> + =A0 =A0char uch;
> + =A0 =A0int i =3D 0;
> +
> + =A0 =A0FILE *fd =3D fopen(lnk_file, "rb");
> + =A0 =A0if (!fd) {
> + =A0 =A0 =A0 =A0error_report("Open file %s failed.", lnk_fi= le);
> + =A0 =A0 =A0 =A0return -1;
> + =A0 =A0}
> + =A0 =A0fread(&flag, 4, 1, fd);
> + =A0 =A0if (flag !=3D 0x4c) {
> + =A0 =A0 =A0 =A0error_report("%s is not a lnk file.", lnk_f= ile);
> + =A0 =A0 =A0 =A0fclose(fd);
> + =A0 =A0 =A0 =A0return -1;
> + =A0 =A0}
> + =A0 =A0fseek(fd, 0x14, SEEK_SET);
> + =A0 =A0fread(&flag, 4, 1, fd);
> + =A0 =A0fseek(fd, 0x4c, SEEK_SET);
> +
> + =A0 =A0if (flag & 0x01) {
> + =A0 =A0 =A0 =A0fread(&sflag, 2, 1, fd);
> + =A0 =A0 =A0 =A0fseek(fd, sflag, SEEK_CUR);
> + =A0 =A0}
> +
> + =A0 =A0offset =3D ftell(fd);
> + =A0 =A0fseek(fd, offset + 0x10, SEEK_SET);
> + =A0 =A0fread(&flag, 4, 1, fd);
> + =A0 =A0fseek(fd, flag + offset, SEEK_SET);
> +
> + =A0 =A0do {
> + =A0 =A0 =A0 =A0fread(&uch, 1, 1, fd);
> + =A0 =A0 =A0 =A0filepath[i++] =3D uch;
> + =A0 =A0} while (uch !=3D '\0');
Please limit the number of bytes to read to capacity of =A0file= path, avoid
the security hole.
@i was limited in [0, MAX_PATH_LEN]= in the new version.
> +
> + =A0 =A0fclose(fd);
> + =A0 =A0return 0;
> +}
> +
> +static long get_win_inode(const char *filename)
> +{
> + =A0 =A0char pbuf[MAX_PATH_LEN], *p;
> + =A0 =A0long inode;
> + =A0 =A0struct stat sbuf;
> + =A0 =A0char path[MAX_PATH_LEN];
> +
> + =A0 =A0/* If filename contains .lnk, it's a shortcuts. Target fi= le
> + * =A0 =A0 =A0* need to be parsed.
> + * =A0 =A0 =A0 =A0 =A0 */
> + =A0 =A0if (strstr(filename, ".lnk")) {
Please be more accurate with this checking, what if the filename happ= ens
to be "a.lnked.txt"?
Generally speaking file= name 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 fil= ename is end with
'.lnk'.
> + =A0 =A0 =A0 =A0if (get_lnk_target_file(filename, path)) {
> + =A0 =A0 =A0 =A0 =A0 =A0error_report("Parse .lnk file %s failed.= ", filename);
> + =A0 =A0 =A0 =A0 =A0 =A0return -1;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0} else {
> + =A0 =A0 =A0 =A0memcpy(path, filename, sizeof(filename));
> + =A0 =A0}
> +
> + =A0 =A0if (stat(path, &sbuf) =3D=3D -1) {
What's this stat() call for?
I read a lot code ab= out 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 so= mething wrong
with this file, the following caculation is useless and return -1 directly.=
> + =A0 =A0 =A0 =A0error_report("get file %s stat error."= , path);
> + =A0 =A0 =A0 =A0return -1;
> + =A0 =A0}
> + =A0 =A0if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) !=3D 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
cou= ld 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.=
> + =A0 =A0 =A0 =A0inode =3D 11003;
> + =A0 =A0 =A0 =A0for (p =3D pbuf; *p !=3D '\0'; p++) {
> + =A0 =A0 =A0 =A0 =A0 =A0inode =3D inode * 31 + *(unsigned char *)p; > + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0return (inode * 911) & 0x7FFF;
> + =A0 =A0}
> +
> + =A0 =A0return -1;
> +}
> +#endif
> +
> =A0static gboolean str_equal_func(gconstpointer a, gconstpointer b) > =A0{
> =A0 =A0 =A0return strcmp(a, b) =3D=3D 0;
> @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char *f= ilename, const char *fmt,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_report("Get file %s stat= failed.", filename);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0#ifdef _WIN32
> + =A0 =A0 =A0 =A0 =A0 =A0inode =3D get_win_inode(filename);
> + =A0 =A0 =A0 =A0 =A0 =A0if (inode =3D=3D -1) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_report("Get file %s inode = failed.", filename);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0#else
Move stat() call here? (seems not necessary for windows)
I decided to move all stat() into get_inode (I changed its name fr= om get_win_inode()). Logic is more simpler now.

Xu Wang
> =A0 =A0 =A0 =A0 =A0 =A0 =A0inode =3D (long)sbuf.st_ino;
> + =A0 =A0 =A0 =A0 =A0 =A0#endif
> =A0 =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0 =A0filename =3D backing_file;
> @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char *f= ilename, const char *fmt,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_report("Get file %s stat= failed.", filename);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
> =A0 =A0 =A0 =A0 =A0}
> +
> + =A0 =A0 =A0 =A0#ifdef _WIN32
> + =A0 =A0 =A0 =A0inode =3D get_win_inode(filename);
> + =A0 =A0 =A0 =A0if (inode =3D=3D -1) {
> + =A0 =A0 =A0 =A0 =A0 =A0error_report("Get file %s inode failed.&= quot;, filename);
> + =A0 =A0 =A0 =A0 =A0 =A0goto err;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0#else
> =A0 =A0 =A0 =A0 =A0inode =3D (long)sbuf.st_ino;
See above.
> + =A0 =A0 =A0 =A0#endif
>
> =A0 =A0 =A0 =A0 =A0if (g_hash_table_lookup_extended(inodes, &inode= , NULL, NULL)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0error_report("Backing file '%s'= ; creates an infinite loop.",
> --
> 1.8.1.4
>
>

--
Fam

--bcaec51dd56db3edce04e186b5ec--