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

[zaytsev]

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.

[andrew_b]

Branch: 2947_quoting_in_user_menu.
changeset:be2883dc86d01fa54003ec75b0a1fc9ed93c9358

[andrew_b]

Merged to master: [be2883dc86d01fa54003ec75b0a1fc9ed93c9358].

[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

[and]

Let's fix it.

[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.

[and]

I vote for remove mc.menu.sr.in

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

[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

[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.

[andrew_b]

Replying to zaytsev:

Therefore, my suggestion is as follows:

Ok, I agree.

[zaytsev]

Ticket created: #3602.

[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.

[zaytsev]

Branch: 2947_quoting_in_user_menu
Initial changeset: [6550d938d1bc2563102a55f039e300516c6e457a]

[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)

[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.

[andrew_b]

Replying to zaytsev:

Branch: 2947_quoting_in_user_menu
Initial changeset: [6550d938d1bc2563102a55f039e300516c6e457a]

Is this branch on review or not?

[zaytsev-work]

@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.

[zaytsev]

Merged to master: [5a39af1372af6dd0685cb81befe2bde4228482c7]

[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

[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.

[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.