From patchwork Sat May 14 00:17:05 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 784602 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4E0HCcl006812 for ; Sat, 14 May 2011 00:17:12 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758428Ab1ENARK (ORCPT ); Fri, 13 May 2011 20:17:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35955 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758423Ab1ENARJ (ORCPT ); Fri, 13 May 2011 20:17:09 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p4E0H7Vd005492 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 13 May 2011 20:17:08 -0400 Received: from tlielax.poochiereds.net (vpn-8-249.rdu.redhat.com [10.11.8.249]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p4E0H7aD016958; Fri, 13 May 2011 20:17:07 -0400 Date: Fri, 13 May 2011 20:17:05 -0400 From: Jeff Layton To: Stefan Metzmacher Cc: linux-cifs@vger.kernel.org Subject: Re: [PATCH] cifs: fix cifsConvertToUCS() for the mapchars case Message-ID: <20110513201705.7fe75309@tlielax.poochiereds.net> In-Reply-To: <1305257555-20656-2-git-send-email-metze@samba.org> References: <1305257555-20656-1-git-send-email-metze@samba.org> <1305257555-20656-2-git-send-email-metze@samba.org> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Sat, 14 May 2011 00:17:12 +0000 (UTC) On Fri, 13 May 2011 05:32:35 +0200 Stefan Metzmacher wrote: > Commit "cifs: fix unaligned accesses in cifsConvertToUCS" > (84cdf74e8096a10dd6acbb870dd404b92f07a756) does multiple steps > in just one commit (moving the function and changing it without testing). > > put_unaligned_le16(temp, &target[j]); is never called for any codepoint > the goes via the 'default' switch statement. As a result we put > just zero (or maybe uninitialized) bytes into the target buffer, > > Signed-off-by: Stefan Metzmacher > --- > fs/cifs/cifs_unicode.c | 20 ++++++++++---------- > 1 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c > index fc0fd4f..b1ff0bd 100644 > --- a/fs/cifs/cifs_unicode.c > +++ b/fs/cifs/cifs_unicode.c > @@ -276,6 +276,7 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen, > return cifs_strtoUCS(target, source, PATH_MAX, cp); > > for (i = 0, j = 0; i < maxlen; j++) { > + charlen = 1; > src_char = source[i]; > switch (src_char) { > case 0: > @@ -315,18 +316,17 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen, > temp = 0x003f; > charlen = 1; > } > - len_remaining -= charlen; > - /* > - * character may take more than one byte in the source > - * string, but will take exactly two bytes in the > - * target string > - */ > - i += charlen; > - continue; > + break; > } > + /* > + * character may take more than one byte in the source > + * string, but will take exactly two bytes in the > + * target string > + */ > put_unaligned_le16(temp, &target[j]); > - i++; /* move to next char in source string */ > - len_remaining--; > + /* move to next char in source string */ > + i += charlen; > + len_remaining -= charlen; > } > > ctoUCS_out: Oof. Yeah, I broke it alright -- good catch... This patch probably won't apply to the current head of the tree, however. Rather than have a special patch for 2.6.38 and .39, what may be best is to go ahead and push 581ade4d in Steve's master branch to stable, and then apply something like this untested patch on top of it. I'll plan to test it out when I get a chance, but I think that's probably the best approach. Thoughts? -----------------[snip]-------------------- cifs: fix cifsConvertToUCS() for the mapchars case As Metze pointed out, commit 84cdf74e broke mapchars case: Commit "cifs: fix unaligned accesses in cifsConvertToUCS" (84cdf74e8096a10dd6acbb870dd404b92f07a756) does multiple steps in just one commit (moving the function and changing it without testing). put_unaligned_le16(temp, &target[j]); is never called for any codepoint the goes via the 'default' switch statement. As a result we put just zero (or maybe uninitialized) bytes into the target buffer. His proposed patch looks correct, but doesn't apply to the current head of the tree. This patch should also fix it. Reported-by: Stefan Metzmacher Signed-off-by: Jeff Layton --- fs/cifs/cifs_unicode.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-) diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c index 23d43cd..1b2e180 100644 --- a/fs/cifs/cifs_unicode.c +++ b/fs/cifs/cifs_unicode.c @@ -277,6 +277,7 @@ cifsConvertToUCS(__le16 *target, const char *source, int srclen, for (i = 0, j = 0; i < srclen; j++) { src_char = source[i]; + charlen = 1; switch (src_char) { case 0: put_unaligned(0, &target[j]); @@ -316,16 +317,13 @@ cifsConvertToUCS(__le16 *target, const char *source, int srclen, dst_char = cpu_to_le16(0x003f); charlen = 1; } - /* - * character may take more than one byte in the source - * string, but will take exactly two bytes in the - * target string - */ - i += charlen; - continue; } + /* + * character may take more than one byte in the source string, + * but will take exactly two bytes in the target string + */ + i += charlen; put_unaligned(dst_char, &target[j]); - i++; /* move to next char in source string */ } ctoUCS_out: