# [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes 5 messages from 2016-12-19 to 2016-12-20. Participants: Max Kirillov, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/44737 ## Max Kirillov, 2016-12-19 21:32 Subject: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes Message-ID: <1482183120-21592-1-git-send-email-max@max630.net> URL: https://gitlist.dev/e/1482183120-21592-1-git-send-email-max%40max630.net ``` UNICODE_STRING::Length field means size of buffer in bytes[1], despite of buffer itself being array of wchar_t. Because of that terminating zero is placed twice as far. Fix it. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx Signed-off-by: Max Kirillov --- Access outside of buffer was very unlikely (for that user needed to redirect standard fd to a file with path longer than ~250 symbols), it still did not seem to do any harm, and otherwise it did not break because only substring is checked, but it was still incorrect. compat/winansi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/winansi.c b/compat/winansi.c index 3be60ce..6b4f736 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -553,7 +553,7 @@ static void detect_msys_tty(int fd) buffer, sizeof(buffer) - 2, &result))) return; name = nameinfo->Name.Buffer; - name[nameinfo->Name.Length] = 0; + name[nameinfo->Name.Length / sizeof(*name)] = 0; /* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */ if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty")) -- 2.3.4.2801.g3d0809b ``` ## Junio C Hamano, 2016-12-19 21:57 Subject: Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes Message-ID: URL: https://gitlist.dev/e/xmqqtw9zmvjq.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1482183120-21592-1-git-send-email-max@max630.net> ``` Max Kirillov writes: > UNICODE_STRING::Length field means size of buffer in bytes[1], despite of buffer > itself being array of wchar_t. Because of that terminating zero is placed twice > as far. Fix it. > > [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx > > Signed-off-by: Max Kirillov > --- Max, I see this is a resend from a few days ago. I suspect that we are in a slow season, so there may be longer delay until we see responses to a patch. I will wait until taking any patch to files specific to MS Windows platform (compat/win*, compat/mingw*, and compat/vcbuild*) without first seeing it reviewed and acked by either of the two Johannes's (well, there might be other people in addition to those two, whose Acked-by/Reviewed-by I should be trusting; if that is the case, it only shows how unqualified I would be as the first contact to be on the To: line of such a patch). I do not mind "see the patch, ping Johanneses as needed, wait and then apply it to my tree" flow, but I also wonder if the process can be made more efficient. Dscho (one of the Johanneses) gets many patches specific to Windows port via the Git-for-Windows project and then "upstreams" by forwarding with his sign-off in my direction, and I do not mind that flow, either. Whichever one is the most efficient for all three parties involved is fine by me, but if one is preferred over the other, perhaps I should write it down in the "notes from the maintainer" or somewhere? Dscho, J6t, what do you think? Thanks. > Access outside of buffer was very unlikely (for that user needed to redirect > standard fd to a file with path longer than ~250 symbols), it still did not > seem to do any harm, and otherwise it did not break because only substring is > checked, but it was still incorrect. > compat/winansi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/compat/winansi.c b/compat/winansi.c > index 3be60ce..6b4f736 100644 > --- a/compat/winansi.c > +++ b/compat/winansi.c > @@ -553,7 +553,7 @@ static void detect_msys_tty(int fd) > buffer, sizeof(buffer) - 2, &result))) > return; > name = nameinfo->Name.Buffer; > - name[nameinfo->Name.Length] = 0; > + name[nameinfo->Name.Length / sizeof(*name)] = 0; > > /* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */ > if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty")) ``` ## Max Kirillov, 2016-12-20 05:21 Subject: Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes Message-ID: <20161220052130.GA10028@wheezy.local> URL: https://gitlist.dev/e/20161220052130.GA10028%40wheezy.local In-Reply-To: ``` On Mon, Dec 19, 2016 at 01:57:29PM -0800, Junio C Hamano wrote: > Max, I see this is a resend from a few days ago Sorry about resend. For some reason I don't get the list copy (might be some clever duplicate elimination in my forwardings), and marc.info seems to be slow to update, so I had no indication the first message got into list. Now I see they are there in the other archive. PS: probably http://vger.kernel.org/vger-lists.html#git should be updated because there is no archive at gmane anymore. -- Max ``` ## Johannes Schindelin, 2016-12-20 15:16 Subject: Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1612201610270.54750%40virtualbox In-Reply-To: <1482183120-21592-1-git-send-email-max@max630.net> ``` Hi Max, On Mon, 19 Dec 2016, Max Kirillov wrote: > UNICODE_STRING::Length field means size of buffer in bytes[1], despite > of buffer itself being array of wchar_t. Because of that terminating > zero is placed twice as far. Fix it. This commit message needs to be wrapped at <= 76 columns per row. > [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx > > Signed-off-by: Max Kirillov > --- > Access outside of buffer was very unlikely (for that user needed to redirect > standard fd to a file with path longer than ~250 symbols), it still did not > seem to do any harm, and otherwise it did not break because only substring is > checked, but it was still incorrect. Very good, thank you for fixing my mistake! I verified locally that it does exactly the right thing with your patch. ACK, Dscho ``` ## Junio C Hamano, 2016-12-20 17:07 Subject: Re: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes Message-ID: URL: https://gitlist.dev/e/xmqqpokmlebc.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > Hi Max, > > On Mon, 19 Dec 2016, Max Kirillov wrote: > >> UNICODE_STRING::Length field means size of buffer in bytes[1], despite >> of buffer itself being array of wchar_t. Because of that terminating >> zero is placed twice as far. Fix it. > > This commit message needs to be wrapped at <= 76 columns per row. > ... > Very good, thank you for fixing my mistake! I verified locally that it > does exactly the right thing with your patch. Thanks, both. I'll queue this like so. -- >8 -- From: Max Kirillov Date: Mon, 19 Dec 2016 23:32:00 +0200 Subject: [PATCH] mingw: consider that UNICODE_STRING::Length counts bytes UNICODE_STRING::Length field means size of buffer in bytes[1], despite of buffer itself being array of wchar_t. Because of that terminating zero is placed twice as far. Fix it. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380518.aspx Signed-off-by: Max Kirillov Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/winansi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/winansi.c b/compat/winansi.c index 3be60ce1c6..6b4f736fdc 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -553,7 +553,7 @@ static void detect_msys_tty(int fd) buffer, sizeof(buffer) - 2, &result))) return; name = nameinfo->Name.Buffer; - name[nameinfo->Name.Length] = 0; + name[nameinfo->Name.Length / sizeof(*name)] = 0; /* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */ if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty")) base-commit: f7f90e0f4f58d493242078d17c0eba41dd3f1f79 -- 2.11.0-416-g1351c11cce ```