bugGNU nano - Bugs: bug #56023, savefile (^S) says...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #56023: savefile (^S) says "[Cancelled]" in nano-4.0 on an ARM

Submitted by:  easyaspi314 <easyaspi314>
Submitted on:  Thu 28 Mar 2019 07:47:47 PM UTC  
 
Severity:  4 - Important Status:  Fixed
Assigned to:  Benno Schulenberg <bens> Open/Closed:  Closed

( Jump to the original submission)

Tue 16 Apr 2019 08:45:57 AM UTC, comment #14: 

The fix has been released in 4.1.  Thanks for reporting.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 03 Apr 2019 02:37:46 PM UTC, comment #13: 

Or, maybe, you could try compiling with more compilers than just GCC...

Enabling -Wextra and doing a scan-build picks up on a bunch of potential bugs including this one, which was picked up on without any extra flags.

Like, I admit I am biased towards Clang.

However, I'm not saying you should only compile with Clang, just try it out once in a while because its warnings are often useful to avoid this nonsense. Clang kinda focuses on warnings.

Plus, it comes with a decent static analyzer in scan-build.

I attached the log of `scan-build make CFLAGS="-Wall -Wextra -O2 -g".` on the unmodified v4.0 release tag. If you run it directly it will create a HTML page you can browse and it will show you the steps.

cppcheck and PVS Studio also picked up on the uninitialized variable usage. (PVS sees NLS _("string wrappers") as variable format strings).

So it isn't "oh I never would've picked up on it" it is "I didn't use good enough tools to check for bugs before I did a release."

-Wall on one compiler is not enough to pick up on even half of the bugs in your code.

(file #46710, file #46711, file #46712)

easyaspi314 <easyaspi314>
Wed 03 Apr 2019 01:03:36 PM UTC, comment #12: 

(For completeness: nor with 'export MALLOC_PERTURB_=234' nor with 'export GLIBC_TUNABLES="glibc.malloc.perturb=234"', nor with several other numbers, can I make ^S fail on a nano from shortly before commit 7ad232d7.  Tested on three machines.  So, even with malloc perturbation, I wouldn't have caught my mistake.)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 31 Mar 2019 10:02:21 PM UTC, comment #11: 

My x86 (32bit) Gentoo machine also has this problem.

Brand Huntsman <brand>
Sun 31 Mar 2019 06:24:06 PM UTC, comment #10: 

Of the machines that I tried nano on (two 64-bit, and three 32-bit), none of them show the problem.  And as far as I know none of them initialize memory to zero.

Hmm...  On my previous machine, I had a setting that caused allocated memory to be initialized to a random value.  What was it called again?  Googling a bit finds: MALLOC_PERTURB_.  I'll have to start using it again; it would probably have caught this issue.

Version 4.1 will be released in a week or two.  There are a few more changes that I want to make.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 31 Mar 2019 04:43:27 PM UTC, comment #9: 

I added a logger and choice is indeed garbage. It just happens to usually be positive on my Mac.

[ response is 248981688 ]

easyaspi314 <easyaspi314>
Sun 31 Mar 2019 03:40:33 PM UTC, comment #8: 

Well either way it is mucking with UB so it is incorrect.

It all depends on the compiler, compiler flags, and the system.

So yeah, we should release 4.0.1 pretty soon because it is definitely a bug.

easyaspi314 <easyaspi314>
Sun 31 Mar 2019 03:25:39 PM UTC, comment #7: 

For the record, this problem was not limited to ARM chips, despite the  commit message and the recently-changed subject here.  I'm running x86_64 (Slackware-current) and was seeing this problem too.  Maybe your x86_64 systems are patched to initialize things to zero by default, but not everyone's are.

David Lawrence Ramsey <dolorous>
Project Member
Sun 31 Mar 2019 11:19:48 AM UTC, comment #6: 

Applied and pushed to master, commit 7ad232d7.

(Although the 'choice = 0' is not needed, I left it in for clarity.)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 30 Mar 2019 10:36:46 AM UTC, comment #5: 

Ow.  I had seen only the original post, not all the activity afterward.  Thanks for the patch, will apply it tomorrow, first some other things to do.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 10:34:44 PM UTC, comment #4: 

Here is the patch. It sets response and choice to zero.

I am just going to put it here, because it is not worth a new issue.

Somehow, GCC didn't even pick up on this one even with -Wuninitialize, so I kind of get why this made it into the release.

GCC's warnings are sometimes lacking (and vice versa with Clang), so I think we should probably do more checking across different compilers and analysis.

(file #46658)

easyaspi314 <easyaspi314>
Thu 28 Mar 2019 10:02:35 PM UTC, comment #3: 

Exactly.  For the record, this was broken back on Feb 20 by commit 0f9d60a.  When that commit split up one variable into two, the initialization of the original variable was lost.

David Lawrence Ramsey <dolorous>
Project Member
Thu 28 Mar 2019 09:21:13 PM UTC, comment #2: 

Wait, Clang literally pointed out the bug for me.

files.c:2131:7: warning: variable 'response' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
                if ((!withprompt || (ISSET(TEMP_FILE) && exiting)) &&
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
files.c:2148:7: note: uninitialized use occurs here
                if (response < 0) {
                    ^~~~~~~~
files.c:2131:3: note: remove the 'if' if its condition is always false
                if ((!withprompt || (ISSET(TEMP_FILE) && exiting)) &&
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
files.c:2104:15: note: initialize the variable 'response' to silence this warning
                int response, choice;
                            ^
                             = 0

Clang clearly doesn't like this code.

Sure enough, initializing it to an unwanted number such as -1 results in a "Cancelled" message.

I'll make a patch.

easyaspi314 <easyaspi314>
Thu 28 Mar 2019 08:41:47 PM UTC, comment #1: 

Update: Just updated Nano on my MacBook to 4.0 from Homebrew and it is not affected.

MacBook Pro (15-inch, Early 2011)/MacBookPro8,2
macOS Mojave 10.14
2.0GHz Intel Core i7 (I7-2635QM, Sandy Bridge)

I am going to try building from source.

easyaspi314 <easyaspi314>
Thu 28 Mar 2019 07:47:47 PM UTC, original submission:  

Platform:
Android 9.0, Termux
LG G3 D851 - Qualcomm Snapdragon 801/Cortex-A15, ARMv7-A (32-bit)
Yes I am weird

I upgraded to Nano 4.0 from the apt repos, and whenever I do ^S on an existing file, all I see is "[Cancelled]" and the file does not save. This only happens on existing files, when saving a new file, it correctly prompts.

This isn't an issue with the terminal capturing ^S or anything, as if I add, say, `bind ^W savefile all` to my nanorc, ^W exhibits the same behavior.

I'm going to try on a few other platforms as well as build from source later, but for now, I just want to bring it up.

easyaspi314 <easyaspi314>

 

Attached Files
file #46710:  pvs.log added by easyaspi314 (5KiB - application/octet-stream)
file #46711:  cppcheck.log added by easyaspi314 (3KiB - application/octet-stream)
file #46712:  scan-build.log added by easyaspi314 (24KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by brand (Posted a comment)
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by dolorous (Posted a comment)
  • -email is unavailable- added by easyaspi314 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-16 bens Open/ClosedOpen => Closed
    2019-04-03 easyaspi314 Attached File- => Added pvs.log, #46710
        Attached File- => Added cppcheck.log, #46711
        Attached File- => Added scan-build.log, #46712
    2019-03-31 bens Severity3 - Normal => 4 - Important
        StatusIn Progress => Fixed
        Assigned toNone => bens
        Summary^S/savefile in Nano 4.0: "[Cancelled]" => savefile (^S) says "[Cancelled]" in nano-4.0 on an ARM
    2019-03-30 bens StatusNone => In Progress
    2019-03-28 easyaspi314 Attached File- => Added 0001-Fix-uninitialized-variable-usage.patch, #46658

    Back to the top


    Powered by Savane 3.5