Probably too late for 2.6.39 for a patch to a non-default mount option
(but I agree that it should go to stable). Thoughts?
Jeff's suggestion below makes sense.
On Fri, May 13, 2011 at 7:17 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 13 May 2011 05:32:35 +0200
> Stefan Metzmacher <metze@xxxxxxxxx> 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 <metze@xxxxxxxxx>
>> ---
>> 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 <metze@xxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> 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:
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
