Re: [PATCH] Use FIX_UTF8_MAC to enable conversion from UTF8-MAC to UTF8

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 21, 2008, 11:04 UTC
Message-ID: <7vejcbzbge.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <fn1sk4$uh4$1@ger.gmane.org>

Mark Junker <mjscod@web.de> writes:

Show 17 quoted lines

> Junio C Hamano schrieb:
>
>> I do not know how Macintosh libc implements "struc dirent", but
>> this approach does not work in general.
>
> IMHO there is no need that this approach works in general because this
> is a fix for MacOSX systems only. I also use d_namlen which might not
> be available on other systems. But on MacOSX this works as expected.
>
>> yet you can obtain a path component longer than 256 bytes.
>> Apparently the library allocates longer d_name[] field than what
>> is shown to the user.
>
> This is not a problem either because on MacOSX we get decomposed UTF8
> and we always convert to composed UTF8. This means that the string
> returned from reencode_string will always be smaller than the original
> filename that had to be reencoded.

It is not quite enough that this works Ok on MacOS, if you made FIX_UTF8_MAC definable in the Makefile. After all some friendly and helpful Linux folks might want to enable it with their build trying to help debugging, right?

In the short term, as long as it safely runs without overrunning the buffer on MacOS, then that is fine, even though we will need some protection to prevent this code from getting compiled and used on Linux with glibc, which does have the issue.

I was specifically talking about this "static" thing.

+static struct dirent temp;

+struct dirent *gitreaddir(DIR *dirp) +{ + size_t utf8_len; + char *utf8; + struct dirent *result; + result = readdir(dirp); + if (result != NULL) { + memcpy(&temp, result, sizeof(struct dirent)); + utf8 = reencode_string(temp.d_name, "UTF8", "UTF8-MAC"); + if (utf8 != NULL) { + utf8_len = strlen(utf8); + temp.d_namlen = (u_int8_t) utf8_len; + memcpy(temp.d_name, utf8, utf8_len + 1); + free(utf8); + result = &temp; + } + } + return result; +}

You memcpy() what the library gave you in *result to the statically allocated "temp". d_name[] in "temp" comes from the structure definition in the user visible include file, which could be much shorter than what the library gave you in *result. The structure definition I showed in my message you are responding to illustrates the issue. If MacOS uses a similar trick to define d_name[256] and sometimes returns much longer name in *result, you are truncating the name by copying only the first part of the structure and first 256 bytes of d_name[].

But you have a Mac, I don't, so as long as you have verified that their header has enough room in statically allocated "temp" to store longest possible name that can be returned from readdir(), the code is Ok. I was just being cautious, as I know the above code has a problem on one platform.