Bug 2112870

Also broken:

coreutils-9.1-3.fc37.x86_64
coreutils-8.32-32.fc34.x86_64
coreutils-8.32-33.fc35.x86_64
coreutils-8.32-32.el9.x86_64


Using LANG=C doesn't change anything.

Thank you for reporting it!  It seems to be caused by coreutils-i18n.patch.

Fedora commit: https://src.fedoraproject.org/rpms/coreutils/c/e0f6afe5e62f80aa5d2007bd725b76158f76222e?branch=rawhide

(In reply to Kamil Dudka from comment #3)
> Fedora commit:
> https://src.fedoraproject.org/rpms/coreutils/c/
> e0f6afe5e62f80aa5d2007bd725b76158f76222e?branch=rawhide

What about simplifying that to:

+            +                  /* mb_width() returns 0 for control characters */            
+            +                  column += MIN(1, mb_width (c));

?

That might result in two calls to mb_width() in the most common case.

Then what about:

const int width = mb_width (c);            
column += MIN(1, width);

?

That works for me.  I found the current version more debugging-friendly as one does not need to step through a macro expansion.  But I have no strong opinion on that.  Martin, thank you for reviewing the change in the first place!

Follow-up Fedora commit: https://src.fedoraproject.org/rpms/coreutils/c/26b2c3ae54184e0be5ce766302769d058c518761?branch=rawhide

Fedora commits:
https://src.fedoraproject.org/rpms/coreutils/c/d559e8c8107f85ebe739b70eaf3e5a030f66aa28?branch=f36
https://src.fedoraproject.org/rpms/coreutils/c/016c9b436824d7827e3d1f1ef5a586e703bc23cc?branch=f35

(In reply to Kamil Dudka from comment #7)
> That works for me.  I found the current version more debugging-friendly as
> one does not need to step through a macro expansion.  But I have no strong
> opinion on that.  Martin, thank you for reviewing the change in the first
> place!

Thanks for taking my suggestion.

FEDORA-2022-536c8337c8 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-536c8337c8

FEDORA-2022-9fc64fd114 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-9fc64fd114

the MIN should have been a MAX(), now the reproducer is again triggering.

Whoops, yeah..

Indeed.  That is embarrassing.  Thank you for letting us know, Dirk!

Follow-up Fedora commits:
https://src.fedoraproject.org/rpms/coreutils/c/4dcaf08c8bbda65a04619f8bc33c585081062d11?branch=rawhide
https://src.fedoraproject.org/rpms/coreutils/c/98f269320269299af57f48d10d45a4d89c576000?branch=f36
https://src.fedoraproject.org/rpms/coreutils/c/69bf2750bbc656f82366e00c0e1442c39dd50276?branch=f35

FEDORA-2022-9fc64fd114 has been pushed to the Fedora 36 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-9fc64fd114`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-9fc64fd114

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-536c8337c8 has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-536c8337c8`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-536c8337c8

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-9fc64fd114 has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.

I got additional feedback on this change. namely expand and unexpand are treated differently in the coreutils-i18n modification. suggest this modification (its a diff of a diff, sorry) on the unexpand case, to match the (simpler) expand implementation:

-@@ -218,9 +277,11 @@ unexpand (void)
+@@ -218,9 +277,9 @@ unexpand (void)
                    next_tab_column = column;
                    tab_index -= !!tab_index;
                  }
-              else
-+              else if (!mb_iseq (c, '\n'))
++              else if (!mb_iscntrl (c))
                  {
 -                  column++;
-+                  /* mb_width() returns 0 for control characters */
-+                  const int width = mb_width (c);
-+                  column += (width) ? width : 1;
++                  column += mb_width (c);
                    if (!column)
                      die (EXIT_FAILURE, 0, _("input line is too long"));
                  }

now the implementation is symmetric to the one in expand().

Thanks for feedback!  I think it makes sense.  iswcntrl('\n') returns true, so we do not need to check for '\n' specifically.

While we are at it, isn't the "input line is too long" check lame in case mb_width() returns value greater than one?

I think this is a check for arithmetic "size_t" overflow, which is defined unlike the signed integer overflow. 
It can not practically happen I'd say, but it is more correct with the check in place.

The type of `column` is in fact uintmax_t.  The check is correct in upstream coreutils because `column` is always incremented by one.  If you take the highest uintmax_t number and increment it by one, the result is zero and the check triggers.  If you take the highest uintmax_t number and add more than one to it, the result is not zero.  I think the check in the i18n patch should actually look like this:

              else if (!mb_iscntrl (c))
                {
                  const uintmax_t orig_column = column;                                       
                  column += mb_width (c);
                  if (column < orig_column)
                    die (EXIT_FAILURE, 0, _("input line is too long"));                       
                }

This avoids the overflow check completely for non-control characters.
This was also the case for older versions of the I18 patches.

What about the following which is closer to the non-I18N code,
i.e., just does the check in a condition-less 'else'?

              else
                {
                  const uintmax_t orig_column = column;
                  /* mb_width returns 0 for control chars which have width 1.  */
                  column += mb_iscntrl(c) ? 1 : mb_width(c);
                  /* Test overflow.  */
                  if (column < orig_column)
                    die (EXIT_FAILURE, 0, _("input line is too long"));
                }

ah, no, my previous suggestion was wrong.

@Kamil: your suggestion in #c23 looks correct.
One suggestion: as mb_width returns 0 for control characters, we don't need to
single them out either, and can simplify to a plain 'else':

              else
                {
                  const uintmax_t orig_column = column;
                  column += mb_width(c);
                  if (column < orig_column)
                    die (EXIT_FAILURE, 0, _("input line is too long"));
                }

Thanks for review, Berny!  I will pick the version from comment #25.

Fedora commit: https://src.fedoraproject.org/rpms/coreutils/c/f3c6ff7e2cdc49208be8add4e51eec3c45767196?branch=rawhide

FEDORA-2022-d2973d1e3b has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-d2973d1e3b

FEDORA-2022-50c9e3fecf has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-50c9e3fecf

FEDORA-2022-50c9e3fecf has been pushed to the Fedora 35 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-50c9e3fecf`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-50c9e3fecf

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-d2973d1e3b has been pushed to the Fedora 36 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-d2973d1e3b`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-d2973d1e3b

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2022-d2973d1e3b has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2022-50c9e3fecf has been pushed to the Fedora 35 stable repository.
If problem still persists, please make note of it in this bug report.