Bug 1324814 - [PATCH] doc fix request for insecure find example
Summary: [PATCH] doc fix request for insecure find example
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: javapackages-tools
Version: 25
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mikolaj Izdebski
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-04-07 11:04 UTC by Pavel Raiskup
Modified: 2017-09-14 10:28 UTC (History)
4 users (show)

Fixed In Version: 5.0.0-7
Clone Of:
Environment:
Last Closed: 2017-09-14 10:28:34 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Fix. (790 bytes, patch)
2016-04-07 11:04 UTC, Pavel Raiskup
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1325049 0 unspecified CLOSED Documentation issue for '-exec {} +' syntax 2021-02-22 00:41:40 UTC

Internal Links: 1325049

Description Pavel Raiskup 2016-04-07 11:04:53 UTC
Created attachment 1144680 [details]
Fix.

Comment 1 Mikolaj Izdebski 2016-04-07 23:27:09 UTC
While I agree that glob character should be quoted, I don't like using -exec for two reasons: 1) it may execute javac more than once if there are many .java files and 2) it always returns true - it won't stop script on compilation failure.

Comment 2 Mikolaj Izdebski 2016-04-07 23:30:49 UTC
Fixed upstream in https://github.com/mizdebsk/javapackages/commit/9e12e1d

Comment 3 Pavel Raiskup 2016-04-08 04:57:27 UTC
(In reply to Mikolaj Izdebski from comment #1)
> While I agree that glob character should be quoted, I don't like using -exec
> for two reasons: 1) it may execute javac more than once if there are many
> .java files and

What limit (when find runs more than one command) are we talking about here?
Probably some limits given by execve()?..  if yes, that is not that important
point as we still pass all the files through execve() later on 'javac' in your
fix.

> 2) it always returns true - it won't stop script on compilation failure.

This sounds like find's documentation issue?  See the bug 1325049.  But this
point really matters, thanks.  I did not know the snippet it is to be used in
scripts ...

... then, it is still very bad shell scripting practice to do 'CMD `CMD`'
in scripts because of white spaces.  In documentation which is to be C&P'ed to
user's scripts people can think it is safe shell pattern..

  $ ls -1
  a b.java
  a.java
  $ javac `find -name '*.java'`
  javac: invalid flag: ./a
  Usage: javac <options> <source files>
  use -help for a list of possible options
  $ find . -name '*.java' -exec javac {} +
  $ echo $?
  0

Comment 4 Mikolaj Izdebski 2016-04-08 06:09:30 UTC
(In reply to Pavel Raiskup from comment #3)
> (In reply to Mikolaj Izdebski from comment #1)
> > While I agree that glob character should be quoted, I don't like using -exec
> > for two reasons: 1) it may execute javac more than once if there are many
> > .java files and
> 
> What limit (when find runs more than one command) are we talking about here?
> Probably some limits given by execve()?..  if yes, that is not that important
> point as we still pass all the files through execve() later on 'javac' in
> your
> fix.

I guess it is smallest from execve limits on systems supported by findutils. It's not even close to limits on Linux.

Try with:
touch $(seq 100000)
find -type f -exec awk 'BEGIN{print"awk executed"}' {} +
awk 'BEGIN{print"awk executed"}' $(find -type f)

All files must be compiled in one invocation

> > 2) it always returns true - it won't stop script on compilation failure.
> 
> This sounds like find's documentation issue?  See the bug 1325049.  But this
> point really matters, thanks.  I did not know the snippet it is to be used in
> scripts ...

Ah, you are right. It looks like it stops when subprocess returns non-zero and propagates that value as its exit code -- this is what I would expect.

> ... then, it is still very bad shell scripting practice to do 'CMD `CMD`'
> in scripts because of white spaces.  In documentation which is to be C&P'ed
> to
> user's scripts people can think it is safe shell pattern..

Java package and class names can't contain whitespaces so it's very unlikely to happen in practice.

The best solution I could think of is:

find -type f -name '*.java' -print0 | sed -z "s/'/'\"'\"'/g;s/.*/'&' /" | javac @/dev/stdin

It doesn't have execve arglist limitations, should work with any file names, but its readability is at least questionable...

Comment 5 Mikolaj Izdebski 2016-04-08 06:22:10 UTC
(In reply to Mikolaj Izdebski from comment #4)
> find -type f -name '*.java' -print0 | sed -z "s/'/'\"'\"'/g;s/.*/'&' /" |
> javac @/dev/stdin
> 
> It doesn't have execve arglist limitations, should work with any file names,
> but its readability is at least questionable...

Actually this won't work for some bad cases. Improved version:

find -type f -name '*.java' -print0 | sed -z 's/\\/\\\\/g;s/\n/\\\n/g;s/"/\\"/g;s/.*/"&" /' | javac @/dev/stdin

Comment 6 Pavel Raiskup 2016-04-08 06:47:46 UTC
Right.  My original point was that people should care about globing and
command substitution while using 'find'.  And we should not show them
potentially wrong examples.

The actual status might be good enough.. it is up to you, I just wanted to
argue why I chose the variant

  'find ... -exec .. {} +'

over "more intuitive", "more "portable"" but "less bulletproof":

  javac `find .. '*.java'`

Comment 7 Jan Kurik 2016-07-26 04:46:12 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 25 development cycle.
Changing version to '25'.

Comment 8 Mikolaj Izdebski 2017-09-14 10:28:34 UTC
I believe that this bug is fixed in javapackages-tools-5.0.0-7,
which is available in Fedora 27, so I am closing this bug now.

The build containing the fix can be found at Koji:
http://koji.fedoraproject.org/koji/buildinfo?buildID=956498

This bug was fixed in the next release of Fedora, and it is currently
not planned to be fixed in the release it was filed against.
You can update to the newer release of Fedora to get the fix.


Note You need to log in before you can comment on or make changes to this bug.