Created attachment 12590 [details] message missing end of message line If a message text file ends before the end of message line (a period on a line by itself), windmc will enter an infinite loop and consuming all available memory until it is killed. This can be caused by either simply leaving out the end of message line, or if it is followed by an EOF instead of a newline. I have attached two sample files that cause the issue, one for each case. I have attached a patch with the most direct solution - checking for the end of input before attempting to read the next line. This causes windmc to exit with the error message 'missing end of message text' instead of looping. While this does not perfectly match the error message of mc.exe, which reports either an invalid character or expected keyword, this seems like a more helpful error message anyway. I'm happy to apply a different fix if there is a better way to go about it, just let me know. Thanks!
Created attachment 12591 [details] message with premature EOF
Created attachment 12592 [details] proposed patch
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8affa48ac7c55ade04713654a22f1c56319b1195 commit 8affa48ac7c55ade04713654a22f1c56319b1195 Author: Joel Anderson <joelanderson333@gmail.com> Date: Fri Jun 5 11:11:03 2020 +0100 Fix a potential infinite loop in the Windows resource parser. PR 26082 * mclex.c (yylex): Add test for an empty input stream.
Hi Joel, Thanks for reporting this bug - and for supplying a fix. I have checked in your patch so I hope that this issue is now resolved. Cheers Nick
Created attachment 12595 [details] 0001-Fix-parse-error-in-the-Windows-resource-parser.patch Patch which fixes the remaining parse error. It was initial added to https://bugzilla.opensuse.org/show_bug.cgi?id=920134
Created attachment 12596 [details] 0002-Fix-catching-EOF-in-the-Windows-resource-parser.patch This patch fixes the remaining, not catched EOF conditions.
I do think that it is important to note that the two new patches alter windmc behavior to accept a file where the period is immediately followed by a EOF instead of a newline or carriage return and newline. These types of files are rejected by current versions of mc.exe, which is what I was using the 'premature EOF' attachment to test against myself. While the new patches are more forgiving to the user and perhaps a better way to handle the scenario, they do not match mc.exe behavior. I can see arguments for both cases, but just want to make sure that it is a deliberate choice to deviate windmc's behavior from that of mc.exe.
(In reply to Joel Anderson from comment #7) > I do think that it is important to note that the two new patches alter > windmc behavior to accept a file where the period is immediately followed by > a EOF instead of a newline or carriage return and newline. Thanks for this pointer. > These types of files are rejected by current versions of mc.exe, Can you say from which version on this behavior was changed ? > which is what I was using the 'premature EOF' attachment to test against > myself. This change means that projects using the old style would have to adjust the corresponding files, which for example requires additional maintenance work for the Mingw maintainers at opensuse to patch all affected packages and report this change to the corresponding projects. > While the new patches are more forgiving to the user and perhaps a better > way to handle the scenario, they do not match mc.exe behavior. > I can see arguments for both cases, but just want to make sure that it is a > deliberate choice to deviate windmc's behavior from that of mc.exe. I think the best would be to extend windmc with an additional command line option to require exact mc compatibility. Projects can use this switch to make sure that only input files compatible to mc are usable. Without this switch, the "windmc" specific extensions would be enabled by default, but a warning is displayed that these files may not be compatible with mc. Project maintainers can then decide whether or not to modify these files accordingly.
Comment on attachment 12596 [details] 0002-Fix-catching-EOF-in-the-Windows-resource-parser.patch Need to be updated
(In reply to Ralf Habacker from comment #8) > Can you say from which version on this behavior was changed ? I just tested with the executable packaged with the Windows SDK for Windows 7 (7.1) and found that it rejects the attached EOF sample. I don't have the time to go farther back, but this suggests that this has been the functionality for at least a very long time. > This change means that projects using the old style would have to adjust the > corresponding files, which for example requires additional maintenance work > for the Mingw maintainers at opensuse to patch all affected packages and > report this change to the corresponding projects. Considering this hasn't been allowed in mc.exe for quite a while (since Windows 7) and resulted in an infinite loop on windmc until this patch, I suspect there aren't many projects relying on it? Unless I have unintentionally broken something which was previously working, in which case I do apologize.
(In reply to Joel Anderson from comment #10) > this suggests that this has been the > functionality for at least a very long time. Thanks for this info. > Considering this hasn't been allowed in mc.exe for quite a while (since > Windows 7) and resulted in an infinite loop on windmc until this patch, > suspect there aren't many projects relying on it? I agree, thanks for your work.
Created attachment 12605 [details] Proposed patch Hi Ralfm Hi Joel, OK, so let me see if I have this right - windmc should reject ".<EOF>" or any line that reaches end-of-file without having a terminating newline character ? If that is right - they what do you think of this reformatted patch ? It is made from the previous two patches, but changed to reject ".<EOF>". Is this what you had in mind ? Cheers Nick
I think that patch looks great! It is more readable than the current HEAD, the intent is much clearer, and it is consistent with mc.exe behavior in both scenarios.
(In reply to Nick Clifton from comment #12) > Created attachment 12605 [details] Hi Nick, looks good to me, except on spelling error in a comment 326-: /* Skip characters in input_stram_pos up to and including a newline 326+: /* Skip characters in input_stream_pos up to and including a newline
Created attachment 12609 [details] 0001-windmc-Reject-EOF-without-line-break.patch Converted attachment 12605 [details] to a git formatted patch. BTW: attachment 12605 [details] was not tagged as patch, so I cannot tag it as "superseeded"
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=25065fcd192d9958c03e107985aea41d651e4a16 commit 25065fcd192d9958c03e107985aea41d651e4a16 Author: Ralf Habacker <ralf.habacker@freenet.de> Date: Wed Jun 10 10:07:26 2020 +0100 Fix the windmc program to conform to the behaviour of mc.exe by rejecting lines that reach end-of-file without a terminating newline character. PR 26082 * mclex.c (yylex): Reject lines that reach end-of-file without a terminating newline character.
Patch applied.