improvement in lib/widget/widget-common/release_hotkey

[lzsiga]

I'd like to suggest a little improvement in function lib/widget/widget-common/release_hotkey that might be useful in preventing/detecting potential "double-free" problems. This change is very limited, affects only two files: widget-common.h and widget-common.c.

header (lines 127-129)

/* release hotkey, free all mebers of hotkey_t */
void release_hotkey_ptr (hotkey_t *hotkey);
#define release_hotkey(hotkeystruct) release_hotkey_ptr(&(hotkeystruct))

program (lines 98-106)

void
release_hotkey_ptr (hotkey_t *hotkey)
{
#define MEMBER_FREE(m) if (hotkey->m) { g_free (hotkey->m); hotkey->m= NULL; }
    MEMBER_FREE (start);
    MEMBER_FREE (hotkey);
    MEMBER_FREE (end);
#undef MEMBER_FREE
}

[andrew_b]

Replying to lzsiga:

"double-free" problems.

Where, when and why?

 #define MEMBER_FREE(m) if (hotkey->m) { g_free (hotkey->m); hotkey->m= NULL; }

First, you don't find MC_PTR_FREE in mc source code, do you? Second, there is no need to test pointer before g_free() call. Take a look at the GLib reference manual or source code.

Apparently won't fix.

[lzsiga]

Hi, Andrew, thank you for you answer.

Let me answer your points:

1. I'm not the one who experienced problems; I found it in a Hungarian forum (​http://hup.hu/node/145123) and saw this piece of code (release_hotkey), which frees (g_frees) the three pointers but doesn't clear them, leaving chance for two double-free situations. (Plus, passing structures as function-parameters instead of structure-pointers is unusual and not recommended in C language)

2. You're right, MC_PTR_FREE would do just as well (if you include lib/util.h):

   void
   release_hotkey_ptr (hotkey_t *hotkey)
   {
       MC_PTR_FREE (hotkey->start);
       MC_PTR_FREE (hotkey->hotkey);
       MC_PTR_FREE (hotkey->end);
   }
   

3. It doesn't hurt either to check the pointer for zero; an unnecessary function-call (especially into a shared lib) is a waste of CPU.

Yours: Lőrinczy Zsigmond

[andrew_b]

Replying to lzsiga:

1. I'm not the one who experienced problems; I found it in a Hungarian forum (​http://hup.hu/node/145123)

Unfotrunately, I cannot read in Hungarian. Google translate give me some not understandable text.

and saw this piece of code (release_hotkey), which frees (g_frees) the three pointers but doesn't clear them, leaving chance for two double-free situations.

Ok, if double free is the root of problem, we should found out where and why it occurs.

How to reproduce this bug step by step?

(Plus, passing structures as function-parameters instead of structure-pointers is unusual and not recommended in C language)

Replacement structure by pointer isn't a problem.

3. It doesn't hurt either to check the pointer for zero; an unnecessary function-call (especially into a shared lib) is a waste of CPU.

And double check isn't waste, is it?

[lzsiga]

Ok, if double free is the root of problem, we should found out where and why it occurs.

I cannot surely say there is "double free" in the code, I only say the current 'release_hotkey' function does nothing to prevent "double free"

How to reproduce this bug step by step?

I'm not the one who experienced problems; I found it in a Hungarian forum.

Replacement structure by pointer isn't a problem.

That's what I'd like to suggest; to make it easier the original name (release_hotkey) would be kept as a macro. [I might not have said yet: nothing else needs to be modified, I did recompile the whole program after the modification.]

And double check isn't waste, is it?

To tell the the truth, I'm not really interested in this sub-question; as I've said, I think MC_PTR_FREE would do perfectly.

Again, thank you for your answer. Best wishes.

[egmont]

Original forum post is roughly: The reporter guy claims that sporadically mc segfaults for him when he selects multiple files with Insert and then hits F8, or when he hits F10. He's seen it with multiple distros (Ubuntu, Debian, Raspbian) and multiple mc versions.

He says he's seen the same being reported in various forums, and that once an mc developer said he couldn't reproduce. The reporter failed to provide any link to this discussion.

The topic is essentially a flamewar over reference counting vs. garbage collection, and does not focus on this mc bug. The intent of the reporter was apparently to complain/flame and not to find a fix.

Someone said there it's ugly to pass a structure (rather than a pointer) to a method. I don't see any problem with it. Might not be the K&R-style C going back to the 60's (I'm not even sure about it), but who cares.

I don't see any point in this refactoring. I'd just close as invalid.

Edit: Unless, of course, there's a proof that this would fix the bug.

[lzsiga]

Hi egmont,
Certainly, I cannot promise this patch fixes any bug (be that related to mc, glib or libc), but I can promise it is what you normally do in C-programming, when you've freed a pointer: set it to zero to avoid potential future problems.

[zaytsev-work]

The reporter guy claims that sporadically mc segfaults for him when he selects multiple files with Insert and then hits F8, or when he hits F10. He's seen it with multiple distros (Ubuntu, Debian, Raspbian) and multiple mc versions.

Actually, now that I'm reading this, I seem to remember that this has happened to me a couple of times in the last few years, but I could never reliably reproduce it and so... seeing how much bugs Andreas has found using sanitizers, I wouldn't be surprised if it was one of them (and who knows how many are still lurking around).

I think this should be closed as invalid or wontfix; instead of "preventing" double frees by zeroing out pointers after free, one should find and fix buggy code, like what Andrew with Andreas are doing.

Re. pointers to structures, to me this sounds like a dubious optimization.

[lzsiga]

Hi guys,

I've just read back to the post where andrew_b mentioned macro MC_PTR_FREE. Could someone please explain me why this macro has been invented, what are its benefits, why is it used in many modules like util.c, args.c, paths.c, search.c?

(Note: in 4.8.11, it was used only in module syntax.c, so its usage was clearly growing in the past years.)

[lzsiga]

Thank you for your efforts, best wishes.