Ticket #362 (closed defect: fixed)

Opened 10 years ago

Last modified 10 years ago

Incorrect usage of shell_escape() and shell_unescape() functions

Reported by: andrew_b Owned by: slavazanko
Priority: blocker Milestone: 4.7
Component: mc-core Version: master
Keywords: commited-master Cc:
Blocked By: Blocking:
Branch state: Votes for changeset:

Description

As shell_escape() as shell_unescape() functions returns new allocated string. That string must be free when uneeded. But there are many incorrect usages of shell_escape() and shell_unescape():

src/complete.c:217
src/complete.c:505
src/complete.c:561

vfs/fish.c:545
vfs/fish.c:548
vfs/fish.c:564
src/file.c:2122
src/file.c:2123
src/command.c:70
src/complete.c:90
src/complete.c:482

Maybe related to #356

Change History

comment:1 follow-up: ↓ 2 Changed 10 years ago by slyfox

src/complete.c:217
src/complete.c:505

Yes. Looks like those leaks were introduced by commit:0450daf56672a75df543c8222353c8042de8c7a1

src/complete.c:561

Does not look like leak (original string is g_free'd)

vfs/fish.c:545
vfs/fish.c:548
vfs/fish.c:564
src/complete.c:90
src/complete.c:482

Yes. Inplace semantic of unescape_buf function was broken after revert.

src/command.c:70

Yep. examine_cd could stricter have (const char *) proto

comment:2 in reply to: ↑ 1 Changed 10 years ago by andrew_b

Replying to slyfox:

src/complete.c:561

Does not look like leak (original string is g_free'd)

Yes, you're right.

vfs/fish.c:545
vfs/fish.c:548
vfs/fish.c:564
src/complete.c:90
src/complete.c:482

Yes. Inplace semantic of unescape_buf function was broken after revert.

src/command.c:70

Yep. examine_cd could stricter have (const char *) proto

path = shell_unescape(path);
Since inplace semantic of shell_unescape() is currently broken, such usage is wrong.

Will we implement the inplace modifying of input buffer in the shell_unescape() function?

comment:3 follow-up: ↓ 4 Changed 10 years ago by slavazanko

  • Keywords review added
  • Status changed from new to accepted
  • Version changed from 4.6.2 to master
  • Owner set to slavazanko

Will we implement the inplace modifying of input buffer in the shell_unescape() function?

IMHO, not needed.

created branch 362_fix_escape_unescape_alloc (parent: master)

Initial commit: changeset:47a49cc96b7bf859f3b3331cc603598fda42c528

Review, please.

And #356 now closed - because that same problem (but raise in different ways).

comment:4 in reply to: ↑ 3 Changed 10 years ago by andrew_b

Replying to slavazanko:

Initial commit: changeset:47a49cc96b7bf859f3b3331cc603598fda42c528

Review, please.

This commit introduced new bug: pointer to freed memory is used in command_completion_function(). Fixed.

In filename_completion_function(), the tilde_expand() was used incorrect. Fixed.

Alsow small optimization have beed performed.

comment:5 Changed 10 years ago by slavazanko

  • Keywords vote-slavazanko added

Good patch.

comment:6 Changed 10 years ago by styx

  • Keywords vote-styx approved added; review removed

comment:7 Changed 10 years ago by slavazanko

  • Keywords commited-master added; vote-slavazanko vote-styx approved removed
  • Status changed from accepted to testing
  • Resolution set to fixed

comment:8 Changed 10 years ago by slavazanko

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.