Ticket #2947 (closed defect: fixed)

Opened 11 years ago

Last modified 8 years ago

Insufficient quoting in "Delete tagged files if a copy exists in the other directory" and wrong message displayed

Reported by: zaytsev Owned by: zaytsev
Priority: minor Milestone: 4.8.16
Component: mc-core Version: master
Keywords: Cc: onlyjob@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Hi,

The original bug is reported here: https://bugs.launchpad.net/ubuntu/+source/mc/+bug/1094324 . In current master for a directory that contains "a b" and "b c", here is the wrong result:

$  /bin/sh /tmp/mc-zaytsev/mcusrU6oudm
%i has no copy in /home/zaytsev/src/mc-test/a2: NOT deleted.
sum: b: No such file or directory
sum: c: No such file or directory
sum: /home/zaytsev/src/mc-test/a2/b: No such file or directory
sum: c: No such file or directory
b c: DELETED.

Expected result:

$  /bin/sh /tmp/mc-zaytsev/mcusrnxyw9a
a b has no copy in /home/zaytsev/src/mc-test/a2: NOT deleted.
b c: DELETED.

A patch to fix this is attached.

Change History

comment:1 Changed 11 years ago by onlyjob

  • Cc onlyjob@… added

comment:2 Changed 11 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review

Branch: 2947_quoting_in_user_menu.
changeset:be2883dc86d01fa54003ec75b0a1fc9ed93c9358

comment:3 Changed 11 years ago by slavazanko

  • Votes for changeset changed from andrew_b to andrew_b slavazanko
  • Branch state changed from on review to approved

comment:4 Changed 11 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b slavazanko to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:5 Changed 11 years ago by andrew_b

  • Status changed from testing to closed

comment:6 Changed 8 years ago by zaytsev

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Resolution fixed deleted
  • Branch state changed from merged to no branch
  • Milestone changed from 4.8.8 to Future Releases

comment:7 Changed 8 years ago by zaytsev

Quoting from Debian bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=594647 :

Many of the commands inside /etc/mc/mc.menu are unquoted or improperly
quoted.  This leads to problems for file names that contain spaces or other
special characters.  Usually, this leads to just failing to operate on a
file, but there's at least one security issue.

If you have a file named "some_long_name -z /etc/passwd something_else.bz2"
and run "convert .bz2 to .gz", you'll end with /etc/passwd removed and
placed into "/etc/passwd.bz2".


To fix those, please quote every use of a file name.  This includes places
that seem to be already quoted, like:

D="`basename %f .tar.gz`"

which needs to be:

D="`basename "%f" .tar.gz`"

Apparently the Ubuntu bug isn't really fixed either:

https://bugs.launchpad.net/ubuntu/+source/mc/+bug/1094324

comment:8 Changed 8 years ago by and

Let's fix it.

Changed 8 years ago by and

comment:9 follow-up: ↓ 12 Changed 8 years ago by zaytsev

Ah, so my mistake was that %{x} is replaced by "x", so it shouldn't be quoted :-/ Now I think I understand the system. Good thinking to copy the docs on top, maybe this will prevent further quoting gaffes.

Another question is shouldn't we kill mc.menu.sr.in ? It hasn't been updated in a while, and now has become really out of sync. If there is a demand for it (which I doubt), we can re-introduce it, but I think then some different / proper i10n solution should be implemented, because keeping the files in sync manually is a no-go.

comment:10 Changed 8 years ago by zaytsev

  • Status changed from reopened to accepted
  • Owner changed from andrew_b to zaytsev
  • Milestone changed from Future Releases to 4.8.16

comment:11 Changed 8 years ago by and

I vote for remove mc.menu.sr.in

Every time I look into this file to get an idea what sr stands for. :)

comment:12 in reply to: ↑ 9 Changed 8 years ago by andrew_b

Replying to zaytsev:

Another question is shouldn't we kill mc.menu.sr.in ? It hasn't been updated in a while, and now has become really out of sync. If there is a demand for it (which I doubt), we can re-introduce it, but I think then some different / proper i10n solution should be implemented, because keeping the files in sync manually is a no-go.

#2996

comment:13 follow-up: ↓ 14 Changed 8 years ago by zaytsev

@andrew_b, I agree that it's a good thing to support localized menus. However, right now, we don't support it. This mc.menu.sr.in is not in any way used by mc. The user is supposed to discover this file somehow and overwrite the default mc.menu if she/he wants. Also, maintaining several files in parallel isn't working, as you can see, mc.menu.sr.in is hopelessly outdated and broken.

Therefore, my suggestion is as follows:

  1. For now, delete mc.menu.sr.in (but it will remain in repository history, so one could easily recover it later if needed) to close this ticket and #2996.
  2. Create a new enhancement ticket to properly support menu localization. I can do this, that's no big deal.

If someone wants to work on it, as in think of how to localize menus, integrate with mc so that the right file is picked automatically according to the locale, and link with Transifex, so that we can get the translations in all languages, then great, perfect, I'm all for it. But I don't see a good reason for keeping a broken and unmaintaned menu file until this happens.

comment:14 in reply to: ↑ 13 Changed 8 years ago by andrew_b

Replying to zaytsev:

Therefore, my suggestion is as follows:

Ok, I agree.

comment:15 Changed 8 years ago by zaytsev

Ticket created: #3602.

comment:16 follow-up: ↓ 18 Changed 8 years ago by zaytsev

@and, regarding set bunzip2 -c and friends; shouldn't these blocks be surrounded with a subshell invocation or something? Otherwise, they'd be clobbering current shell arguments to archivers, won't they? Just wondering if I understand this correctly.

comment:17 follow-up: ↓ 20 Changed 8 years ago by zaytsev

Branch: 2947_quoting_in_user_menu
Initial changeset: [6550d938d1bc2563102a55f039e300516c6e457a]

comment:18 in reply to: ↑ 16 Changed 8 years ago by and

Replying to zaytsev:

@and, regarding set bunzip2 -c and friends; shouldn't these blocks be surrounded with a subshell invocation or something? Otherwise, they'd be clobbering current shell arguments to archivers, won't they? Just wondering if I understand this correctly.

Its fine here

# set cmd -c
# echo $1
cmd
# echo $2
-c

per case select will override default "set gzip -cd" (set ealier)
and expected for $1 and $2 before final tar -x

By the way, was there not a patch for xz support?
If not I will prepare a patch after this #2947 commit goes into upsteam.

Common sense is to support gzip always and best compressor of the current time™ (xz is a survivor of bzip2, prominent example is www.kernel.org)

comment:19 Changed 8 years ago by zaytsev

per case select will override default "set gzip -cd" (set ealier)

You are right, but that's not what I meant; I was thinking that resetting $1 and $2 might not be a good thing in general, but apparently it's not a problem; at least I've tried this menu item just now and $1 & $2 in my subshell were not affected.

By the way, was there not a patch for xz support?

I think there were many and most were integrated, but not for the user menu.

comment:20 in reply to: ↑ 17 Changed 8 years ago by andrew_b

Replying to zaytsev:

Branch: 2947_quoting_in_user_menu
Initial changeset: [6550d938d1bc2563102a55f039e300516c6e457a]

Is this branch on review or not?

comment:21 Changed 8 years ago by zaytsev-work

  • Branch state changed from no branch to on review

@andrew_b, sorry, I was waiting for the Travis build to finish, and, in the end, forgot to put it on review, even though the build succeeded.

comment:22 Changed 8 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:23 Changed 8 years ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:24 Changed 8 years ago by and

Hi,
OT but regarding Travis.
How about other build tester plattforms, like build.opensuse.org ?

Example of mc (in light of SuSE Maintainer)
https://build.opensuse.org/package/show/Base:System/mc

Example of fish (direct maintainer)
https://build.opensuse.org/project/show/shells:fish:nightly:master

Requirement: a rpm spec

comment:25 Changed 8 years ago by zaytsev

@and, in general, I don't mind, but given the small amount of time I can contribute to mc, I don't think that it brings enough value to invest the effort in setting up package builders and take care of them in the long term. It's only useful when you are checking the builds and fixing the issues timely, isn't it? Besides, we already have someone building releases on OBS, and another guy is doing Debian packages both nightly and every release (see the wiki:Binaries page).

Also, our Travis setup can be further improved, e.g. one could also run some static analyzers, like cppcheck or what not, build with sanitizers, etc. The problem is time, as usual. You saw for yourself that I was the only one taking care of CI for the last 6+ years, and as my build host was aging nobody took care of setting up Travis, before I finally managed to find time to do it, although everyone was always ready to give me tons of valuable advice.

Finally, you see all this endless whining going on the mailing lists that we are not taking care of tickets fast enough. If I will work more on CI at the expense of patches, I bet there will be even more stink... I don't know what's the way out, maybe we should just shut down the mailing lists, what do you think ;-) ?

By the way, thank you for all your ongoing work on mc. I'm sorry if we are slow to review it, and sometimes we disagree on the solutions. It is still highly appreciated.

comment:26 Changed 8 years ago by and

Thanks for replying Yury,
no criticism regarding work flow and speed. I'm fine with "as long as it takes".

Mostly I'm interested in getting mc in a good shape of compiling/build process
and less in new features.

After next release I will attach configure/build patches for review
and ongoing cleanup patches of course.

comment:27 Changed 8 years ago by andrew_b

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