Repository navigation
Charset handling buggy and extremely complicated #3257
Description
Important
This issue was migrated from Trac:
| Origin | https://midnight-commander.org/ticket/3257 |
|---|---|
| Reporter | egmont (@egmontkob) |
With --enable-charset, set your terminal and locale to some 8-bit non-ISO (e.g. KOI8-* CP* or Windows*), and in mcview set the file's encoding to the same.
Chars 128-159 are replaced by dot, even though these are printable characters in these encodings.
mcview (plain.c) calls is_printable() which in turn calls is_8bit_printable() which calls (assuming xterm-like emulator) is_iso_printable which hardwires that 128-159 are non-printable.
It's a faulty design: the method is_printable() tries to answer the question without having a clue about the character set.
The underlying cause is a very complicated code where there are micro-branches handling 5 different cases. E.g. from mcview plain.c:
#ifdef HAVE_CHARSET
if (mc_global.utf8_display)
{
if (!view->utf8)
c = convert_from_8bit_to_utf_c ((unsigned char) c, view->con
if (!g_unichar_isprint (c))
c = '.';
}
else if (view->utf8)
c = convert_from_utf_to_current_c (c, view->converter);
else
{
c = convert_to_display_c (c);
if (!is_printable (c))
c = '.';
}
#else /* HAVE_CHARSET */
if (!is_printable (c))
c = '.';
#endif /* HAVE_CHARSET */
It's very complicated and error prone.
The 5 totally separate cases are:
- mc with charset support:
- (1) file is utf8, display is utf8
- (2) file is utf8, display is 8bit
- (3) file is 8bit, display is utf8
- (4) file is 8bit, display is 8bit (but they can be different encodings, a 256-byte conversion table is used)
- (5) mc without charset support (bytes from the file are directly passed thru to the terminal)
This is not the only place where such a terrible #ifdef and if() branching is done, there are plenty, they are hard (practically impossible) to test, and they are apparently buggy.
This approach also implies that you cannot use handy methods like g_unichar_whatever anywhere outside of such ifdef/if's because basically whenever you have an "int c", you have no clue which character it represents. Even figuring out if it's printable would need to carry its charset as extra context, defeating the purpose of Unicode. (In #3250 I often need to check things like whether the character is CJK or combining, these influence line wrapping and such. I can't do with a single g_unichar_xx() call, I need to write helper methods for each desired g_unichar_xx() with #ifdef/if()s.)
My recommendation is to simplify to at most 2 cases (by merging (1)-(4)), but preferably to only 1 case (by removing (5) if feasible), by forcing Unicode internally.
This could be done if instead of a 1-phase conversion we went for a 2-phase conversion. Most data has the lifecycle that first we receive it from some source of arbitrary encoding (e.g. file name, file contents), then process it, then finally display it (in some other arbitrary encoding). Doing a one-shot conversion means that no matter where you do it, your data might be in any of multiple encodings while you process it and you'd need to carry that encoding and do micro-branches all the time.
Instead, the data should be converted to Unicode as soon as we read it, so that processing always happens in Unicode, and should be converted to the terminal's charset as the last step just when we print it.
Of course, in this approach if the terminal encoding and the file encoding are the same, you do a back-n-forth conversion. But, in turn, the whole code becomes cleaner and less buggy.
About the 5th case: unless there are systems which don't support --enable-charset and yet we'd still want mc to run on them, I'd vote for ditching this possibility.
Nowadays when every new system is set up with default UTF-8 locale, (1) is I assume by far the most frequent use case. The desired change wouldn't introduce any runtime cost with (1)-(2), what happens inside mc would mostly stay the same. With (3)-(4)-(5) we'd perform some operations (e.g. search in file) in Unicode rather than 8-bit. In cases (4)-(5) we'd do an extra back-n-forth conversion, I think this is a price we can pay for having a source that's easier to maintain and test, and has fewer bugs.
Activity
mc-butler commentedon Aug 28, 2014
There should also be a clear distinction between a character being printable by definition, and a character being representable in the current display charset. There might be cases where they should be treated differently.
E.g. I intentionally decided not to allow nroff formatting for non-printable characters. If 'x' is a non-printable Unicode character, "_\bx" is formatted as "_.." (a replacement character for the backspace and a replacement character for the non-printable one). If 'x' is printable, but can't be converted to the display's charset, the desired rendering is an underlined ".".
mc-butler commentedon Jan 3, 2016
Since printable chars are locale-dependent, is_printable should be a member of str_class.
mc-worker commentedon Mar 16, 2025
This is the FAR way and I don't like it.
mcedit is byte accurate. It converts key input and screen output, internally holding the file content as original byte sequence. This is the key feature, and I don't want to lose it.
zyv commentedon Mar 16, 2025
I'm going to abuse this ticket to ask you both @aborodin and @egmontkob a stupid question:
Why do we need an editor and a viewer at all? Can't we use the same code base for editing and viewing, with only the viewing being read-only?
I understand that editor is now based on ropes, but wouldn't the same design work for viewer?
egmontkob commentedon Mar 16, 2025
Great question. I don't know why we have two completely different solutions. My guess is legacy: probably Norton / Volkov also worked in the same way.
Here's another difference that you didn't think of.
Scalability.
As far as I know, the viewer allows you to open enormous binary files, such as even
/dev/hda1or alike, and handle them pretty effectively (even including operations like jumping to the end).For an editor, and for syntax highlighting, it's practically inevitable that you have to load it all into memory, the overall file size has to be reasonable (i.e. some perf degradation is probably acceptable if you have way too many lines), and the same goes for line length.
Another subtle difference arising from the desire of the viewer being able to handle arbitrary large files is that it cannot display the line number.
It's always been unclear to me why the viewer has, indeed, a hex editor; but I think it can only overwrite indivial bytes in place, it cannot delete or insert a byte (shifting the rest of the file), so it's not a general purpose editor.
Also the viewer support manpage syntax, this is not syntax highlighting per se but a similar concept from a bird's eye view.
The two having common grounds would surely be nice in some aspects; e.g. it would surely be great for the viewer to do syntax highlighting. But the advantages and the focuses of the two components are elsewhere; the closer we move them, the more codebase they share, chances are that the individual strengths of the two components could not be easily maitained.
aborodin commentedon Mar 16, 2025
mc-worker commentedon Mar 19, 2025
What about remove
--enable/--disable-charsetand provide charset conversions unconditionally? Any disadvantages?egmontkob commentedon Mar 19, 2025
Not sure: what does that option do exactly?
My gut feeling is: with "disable", mc/mcedit/mcview does a "pass thru", that is, each byte encountered in the file becomes a byte output to the terminal, without understanding what it is. If so, it's high time to remove that option.
Both for the interpretation of a user data (e.g. file name, file contents), and both for the underlying terminal emulator, mc/mcedit/mcview has to know their encoding and convert back-n-forth accordingly.
Related: #1612.
This bug here is 10 years old, that one is 15. Every reasonable system should have migrated to UTF-8 by now. Interpreting any file contents without knowing its charset, or talking to a terminal without knowing its charset, is a faulty concept that should no longer exist.
In fact, there are plenty of new modern terminal emulators out there that only support UTF-8 and no one is complaining about it. I wouldn't mind if mc also dropped support for terminal encodings other than UTF-8. This could also result in the removal of some code, since there'd be no more need to handle the situation where mc understand what letter it should display but is unable to do so.
As usual, my take on this feature removal: If anyone needs mc on a system that is decades behind today's standards, they are more than welcome to use an older version.
(Note that I'm not suggesting to remove the feature of interpreting a file's contents in a wide range of supported charsets. That should remain for another 10-15 years I guess.)
zyv commentedon Mar 20, 2025
I've looked into it and I think the story is as follows:
Sometime around 67a1189, patches were accepted that would allow files to be converted from source encoding to display encoding.
The patches relied on a working
iconvimplementation. Therefore, a compile option was introduced, since not many systems in ~2000 would have a perfectly working iconv.Today, we (hopefully) rely entirely on glib. What they do to give us a working implementation is none of our business (may be system implementation or something else).
So now this option doesn't make any sense and is even harmful in my opinion. Therefore, I absolutely support the idea of removing it.
zyv commentedon Mar 20, 2025
Actually, I don't care and just use what the system is set up for.
If I did care, I think I would actually use editor as a pager if:
... which probably answers my original question.
Now I realize that I think we're loading the entire file into memory in the editor, and the viewer can (at least in theory) handle arbitrarily large files.
Yes, I actually do love this feature of mcedit.
I mean the design of the internal data structure that represents the text to be edited or viewed.
I have to admit that I have never studied this in detail. So if you did, and you say there's no good internal representation that satisfies all the requirements, I'd have to believe you (or check it myself).
My point is that I wish we could share as much code as possible between the two components and have them at feature parity without having to maintain two completely different implementations. I often find myself using the editor when I want to use the viewer just for syntax highlighting.
Yes, there is a ticket #28 for that. But how does it help?
9 remaining items
mc-worker commentedon Aug 1, 2026
What screen corruption are you talking about?
#4668 should be fixed in this branch.
I think no.
Fixed. All introduced parentheses around
-1are removed.Ticket #3257: unification and simplification of character set convers…
zyv commentedon Oct 24, 2026
Looking at the new code, it appears to me that there are still some places where
'.'is used instead ofNON_PRINTABLE_CHAR.mc-worker commentedon Oct 25, 2026
All
'.'that marks non-printable characters are replaced withNON_PRINTABLE_CHAR.Ticket #3257: tests: add tests for convert_char_to_display()
Ticket #3257: tests: add tests for text parsing in mcviewer
Ticket #3257: tests: add tests for edit_move_forward3()
zyv commentedon Oct 25, 2026
I've had an agent run exhaustive tests on this branch, and it uncovered a SIGSEGV and a hang, as well as a number of regressions against master plus two pre-existing bugs.
The issues #4668 and #4880 are now fixed for good, but #3257 itself and #4779 need character classification in the conversion engine. Additionally, there is now one iconv call per byte in
edit_move_forward3()- I'm not sure how critical the resulting ~10x slowdown is.Below is the finding table to accompany the fixups, tests, and bug fixes. I'll send the rest per email.
Hope that helps.
hexedit_mode: heap use-after-free and SIGSEGV on exit with a viewer in the background, losing unsaved buffers of other editors; unsaved hex edits discarded without a prompt once edit mode is toggled off84b1266char_length == 0at EOF)32034cc84b12665bcfffcmcview_is_nroff_sequence()returns -1 (TRUE): the coordinate cache never grows, so Goto line and open-at-line don't work beyond the first 1 KiB5bcfffc_, bold+underline not recognized, non-sequences show the wrong character32034cc32034ccbe008d2,0a747e4UNKNCHARcounted as^A)4e8f8754e8f875d84a625ÀÁÂfor CP1251АБВ)32034cc32034cce.x ..abinstead ofex ....ab)32034cc.instead of U+25CC; spacing mark after a combining mark silently dropped32034ccdpy_wrap_dirty)d2ade0f99b44d4does not compile (dview->convconv): breaks bisect99b44d4mcview_nroff_seq_info()dereferencesnroffbefore its NULL check32034ccedit_move_forward3()15 to 20x slower per character for 8-bit files on a UTF-8 terminal (iconv per byte on a hot path)4e8f875.on CP866/KOI8/CP125x terminalsmcview_nroff_seq_prev()reads the wrong index in its second loop (backward nroff search)dview_get_utf()returns an unsigned byte)Severity: Critical = crash, hang or data loss; High = core function broken or wrong file offset; Medium = wrong display or cursor position in common cases; Low = cosmetic, edge case, or build/bisect hygiene. "Introduced in" is the branch commit that the fixup amends.
ossilator commentedon Oct 26, 2026
i suspect the slowdown to be irrelevant in this place, but it might point to a pre-existing problem in more relevant places. would it be feasible to make the abstraction work on hunks (size externally determined, likely by the display width) rather than characters?