[review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Florian Weimer (Code Review)]

Florian Weimer has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445
......................................................................


Patch Set 2:

This change is ready for review.


-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I5b28fb36c8d224cf69910fb436ff76bff8f129f6
Gerrit-Change-Number: 445
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Wed, 30 Oct 2019 10:32:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Florian Weimer]

* Florian Weimer:

> Florian Weimer has posted comments on this change.
>
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445
> ......................................................................
>
>
> Patch Set 2:
>
> This change is ready for review.

That is not right because the patch is missing from the email
notification. 8-(

I submitted the patch as a WIP, to see what it would look like in
Gerrit.  I think this staging is quite useful as a feature.

The manual says this:

| Alternatively, click Ready from the Change screen.

<https://gnutoolchain-gerrit.osci.io/r/Documentation/intro-user.html#wip>

But I do not see this button.  I clicked “Start Review” instead,
resulting in the notification above and a spurious v2 being created.
(Or maybe that happened because I edited the commit message in Gerrit?)

Thanks,
Florian

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Sergio Durigan Junior]

On Wednesday, October 30 2019, Florian Weimer wrote:

> * Florian Weimer:
>
>> Florian Weimer has posted comments on this change.
>>
>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445
>> ......................................................................
>>
>>
>> Patch Set 2:
>>
>> This change is ready for review.
>
> That is not right because the patch is missing from the email
> notification. 8-(
>
> I submitted the patch as a WIP, to see what it would look like in
> Gerrit.  I think this staging is quite useful as a feature.
>
> The manual says this:
>
> | Alternatively, click Ready from the Change screen.
>
> <https://gnutoolchain-gerrit.osci.io/r/Documentation/intro-user.html#wip>
>
> But I do not see this button.  I clicked “Start Review” instead,
> resulting in the notification above and a spurious v2 being created.
> (Or maybe that happened because I edited the commit message in Gerrit?)

Hm, indeed, maybe the documentation is outdated.  I am also seeing a
"Start Review" button here.

I thought it could be something related to lack of permissions, so I did
a small adjustment and add the necessary permission to the access list,
but I still don't see a "Ready" button.

I wonder what would happen if you'd done a:

  git push gerrit HEAD:refs/for/master%ready

as the documentation says...  I'll submit a patch for review soon, and
I'll give this a try.

Anyway, it is strange because gerrit should have sent a "new change"
notification when you edited the commit message.  Instead, it just sent
a "comment" notification.

If you go the web interface, you can see that the difference between v1
and v2 is exactly the commit message.  Something else that might have
happened is gerrit not sending a "new change" notification when just the
commit message is edited (although I find this unlikely).

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Simon Marchi]

On 2019-10-30 6:36 a.m., Florian Weimer wrote:
> That is not right because the patch is missing from the email
> notification. 8-(
> 
> I submitted the patch as a WIP, to see what it would look like in
> Gerrit.  I think this staging is quite useful as a feature.
> 
> The manual says this:
> 
> | Alternatively, click Ready from the Change screen.
> 
> <https://gnutoolchain-gerrit.osci.io/r/Documentation/intro-user.html#wip>
> 
> But I do not see this button.  I clicked “Start Review” instead,
> resulting in the notification above and a spurious v2 being created.

See the paragraph just below in the doc, which starts with "In the new
PolyGerrit UI".  The PolyGerrit UI is the newer UI that we use.  I suppose
they should change those sentences, because the old UI isn't available
in Gerrit 3 (which this doc targets).

I filed this bug: https://bugs.chromium.org/p/gerrit/issues/detail?id=11837

> (Or maybe that happened because I edited the commit message in Gerrit?)

Indeed, the v2 is because you have updated the commit message in Gerrit, we
can see the diff between v1 and v2 here:

https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445/1..2//COMMIT_MSG

Editing the commit message directly on Gerrit is the same as if you'd edit
it locally and pushed the patch again, it creates a new version of the patch.

Simon

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Simon Marchi]

On 2019-10-30 11:54 a.m., Sergio Durigan Junior wrote:
> Anyway, it is strange because gerrit should have sent a "new change"
> notification when you edited the commit message.  Instead, it just sent
> a "comment" notification.
> 
> If you go the web interface, you can see that the difference between v1
> and v2 is exactly the commit message.  Something else that might have
> happened is gerrit not sending a "new change" notification when just the
> commit message is edited (although I find this unlikely).

I suppose if he did the operations in this order:

1. Edit the commit message (which creates v2)
2. Make the change ready for review

Then when v2 is created, the change is still WIP, so no notification is sent.

Then, step #2 is akin to leaving a global comment on the change, which doesn't
include the full diff (because presumably the full diff would have been sent
before).

I think we need to check if there's the possibility of generating a notification
with the full diff when the wip -> ready state transition happens.

Simon

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Florian Weimer]

* Simon Marchi:

>> (Or maybe that happened because I edited the commit message in Gerrit?)
>
> Indeed, the v2 is because you have updated the commit message in Gerrit, we
> can see the diff between v1 and v2 here:
>
> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445/1..2//COMMIT_MSG
>
> Editing the commit message directly on Gerrit is the same as if you'd edit
> it locally and pushed the patch again, it creates a new version of the patch.

Hmm.  I'm surprised that there are versions for WIP patches.  In any
case, the first review after leaving WIP status should probably mail out
all the diffs.

The present state makes the WIP feature rather useless IMHO because
despite the WIP status, it seems that you are stuck with what you put
into Gerrit.

Thanks,
Florian

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Simon Marchi]

On 2019-10-30 12:07 p.m., Florian Weimer wrote:
> * Simon Marchi:
> 
>>> (Or maybe that happened because I edited the commit message in Gerrit?)
>>
>> Indeed, the v2 is because you have updated the commit message in Gerrit, we
>> can see the diff between v1 and v2 here:
>>
>> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445/1..2//COMMIT_MSG
>>
>> Editing the commit message directly on Gerrit is the same as if you'd edit
>> it locally and pushed the patch again, it creates a new version of the patch.
> 
> Hmm.  I'm surprised that there are versions for WIP patches.  In any
> case, the first review after leaving WIP status should probably mail out
> all the diffs.
> 
> The present state makes the WIP feature rather useless IMHO because
> despite the WIP status, it seems that you are stuck with what you put
> into Gerrit.

Why should WIP patches not have versions?  If you upload a few versions of the
same change, can't it be useful to go back to a previous upload?

Simon

Re: [review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Sergio Durigan Junior]

On Wednesday, October 30 2019, Simon Marchi wrote:

> On 2019-10-30 11:54 a.m., Sergio Durigan Junior wrote:
>> Anyway, it is strange because gerrit should have sent a "new change"
>> notification when you edited the commit message.  Instead, it just sent
>> a "comment" notification.
>> 
>> If you go the web interface, you can see that the difference between v1
>> and v2 is exactly the commit message.  Something else that might have
>> happened is gerrit not sending a "new change" notification when just the
>> commit message is edited (although I find this unlikely).
>
> I suppose if he did the operations in this order:
>
> 1. Edit the commit message (which creates v2)
> 2. Make the change ready for review
>
> Then when v2 is created, the change is still WIP, so no notification is sent.

Ah, you're right, it seems that he did it in this order indeed.

> Then, step #2 is akin to leaving a global comment on the change, which doesn't
> include the full diff (because presumably the full diff would have been sent
> before).

Right.

> I think we need to check if there's the possibility of generating a notification
> with the full diff when the wip -> ready state transition happens.

I think this is related: https://bugs.chromium.org/p/gerrit/issues/detail?id=8364

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

[review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- wcsmbs/wcsrtombs.c
| +++ wcsmbs/wcsrtombs.c
| @@ -98,17 +98,29 @@ #endif
|      }
|    else
|      {
| -      /* This code is based on the safe assumption that all internal
| -	 multi-byte encodings use the NUL byte only to mark the end
| -	 of the string.  */
| -      const wchar_t *srcend = *src + __wcsnlen (*src, len) + 1;
|        size_t dummy;
|  
| +      /* This code is based on the safe assumption that all internal
| +	 multi-byte encodings use the NUL byte only to mark the end of
| +	 the string.  It also assumes that every wide character
| +	 produces at least one byte of output.  This is true for all
| +	 supported locales, including the very special case of the
| +	 Big5 character set.  But custom locales (e.g., for TSCII)
| +	 might not have this property.
| +
| +	 Do not use wcsnlen because the input may not be an array of
| +	 len wchar_t elements, given that len is the length of the
| +	 *destination* array.  */
| +      const wchar_t *srcend = *src;
| +      while (srcend < *src + len && *srcend != L'\0')
| +	++srcend;
| +      ++srcend;

PS2, Line 117:

OK. This looks good to me. We scan src looking for L'\0' rather than
using wcsnlen which is undefined because len may be longer than src's
real length.

| +
|        data.__outbuf = (unsigned char *) dst;
|        data.__outbufend = (unsigned char *) dst + len;
|  
|        status = DL_CALL_FCT (fct, (tomb, &data, (const unsigned char **) src,
|  				  (const unsigned char *) srcend, NULL,
|  				  &dummy, 0, 1));
|  
|        /* Count the number of bytes.  */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I5b28fb36c8d224cf69910fb436ff76bff8f129f6
Gerrit-Change-Number: 445
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 19:12:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]

[Florian Weimer (Code Review)]

Florian Weimer has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/445 )

Change subject: wcsrtombs: Do not call wcsnlen on input with wrong length [BZ #23711]
......................................................................


Abandoned

This use of wcsnlen looks like a GNU extension.
-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I5b28fb36c8d224cf69910fb436ff76bff8f129f6
Gerrit-Change-Number: 445
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: abandon