After the audit, I had this noted down as a TODO-item, but
considered the function to be tested enough to hold the line
until I came to rewrite it.
Admittedly, I didn't take a closer look at the previous loop
and there probably were some edge-cases which caused trouble, but
so far so good, the new version of this commit should be safe
and considered audited.
1) Refactor the manpage with num-options, optimize wording to be more
concise and to the point, pid also specifies process groups.
2) Make int sig const.
3) Remove prototypes.
4) /* not reached */ consistency.
5) Refactor usage() with eprintf.
6) Refactor arg-parser with a switch, use estrtonum
7) Use return instead of exit() in main()
8) argc-argv-correctness.
1) Use num-wording in the manpage, remove offensive remark against
the beloved -num-syntax <3.
2) Style changes.
3) Report errors of getline.
4) argv-argc-centric argument loop.
5) Rename r to ret for consistency.
We just take the raw argument list as is. Using arg.h, arguments
beginning with - would have been "eaten up".
Writing a special "bailout" for arg.h was not a good option,
not because it's not impossible (done in 6 LOC), but because it
is a shoehorning around a corner case present for a few programs
which are broken by design by POSIX.
1) Any path passed to mkdir -p beginning with '/' failed, because
it would cut out the first '/' immediately, passing "" to mkdir.
2) Running mkdir -p with a path/to/dir without trailing '/' would
not create the directory.
This is due to a wrong flag-check I added in the main-loop.
It should now work as expected.
3) With the p-flag given, don't report an error in case the last
dir also exists.
For loop detection, a history is mandatory. In the process of also
adding a flexible struct to recurse, the recurse-definition was moved
to fs.h.
The motivation behind the struct is to allow easy extensions to the
recurse-function without having to change the prototypes of all
functions in the process.
Adding flags is really simple as well now.
Using the recursor-struct, it's also easier to see which defaults
apply to a program (for instance, which type of follow, ...).
Another change was to add proper stat-lstat-usage in recurse. It
was wrong before.
While auditing du(1) I realized that there's no way the over 100 lines
of procedures in du() would pass the audit.
Instead, I decided to rewrite this section using recurse() from libutil.
However, the issue was that you'd need some kind of payload to count
the number of bytes in the subdirectories and use them in the higher
hierarchies.
The solution is to add a "void *data" data pointer to each recurse-
function-prototype, which we might also be able to use in other
recurse-applications.
recurse() itself had to be augmented with a recurse_samedev-flag, which
basically prevents recurse from leaving the current device.
Now, let's take a closer look at the audit:
1) Removing the now unnecessary util-functions push, pop, xrealpath,
rename print() to printpath(), localize some global variables.
2) Only pass the block count to nblks instead of the entire stat-
pointer.
3) Fix estrtonum to use the minimum of LLONG_MAX and SIZE_MAX.
4) Use idiomatic argv+argc-loop
5) Report proper exit-status.
1) Add check to parselist() to warn about an empty list.
2) Remove all "cut: "-prefixes from error-messages and other style
changes.
3) != -1 --> >= 0 and check for ferror on fp after getline.
4) Update usage with argv0.
5) argv-centric loop refactor
6) Properly report exit-status.
7) Add empty line before return.
1) Use the LIMIT()-macro in util.h instead of defining our own.
2) Drop nextline() and finish(), not needed anymore. Use
fputs in printline instead of printf.
--> BUGFIX: Finish exited with status 1, but actually should
exit with status 0 if ferror(f) == 0.
3) Don't use /dev/fd/0 and use idiomatic <stdin> and fp = stdin
instead.
4) Refactor loop to use getline() instead of some handrolled
nextline-function.
--> BUGFIX: Line-length was limited to LINE_MAX before, now
it's factually unlimited.
5) Combine diff >= 0 and diff <= 0 into one loop with a beginning
continue-condition (diff && i == (diff < 0)).
6) BUGFIX: If diff == 0, don't print one buffer after EOFing on the
other.
1) Remove the return-value-enum, which is not necessary for a simple
program like this.
2) Don't disallow both l and s to be specified. This is undefined
behaviour defined by POSIX, so we don't start demanding things
from the user.
3) Replace exit() with return (we are in main).
4) Refactor main loop to never return in the loop, but actually
set the same-value and break, which increases readability.
5) Remove the final fclose()'s. The OS will take care of them, no
need to become cleansy here.
6) Use idiomatic return-value using same. This concludes the
increase of readability in the main-loop.
After a short correspondence with Otto Moerbeek it turned out
mallocarray() is only in the OpenBSD-Kernel, because the kernel-
malloc doesn't have realloc.
Userspace applications should rather use reallocarray with an
explicit NULL-pointer.
Assuming reallocarray() will become available in c-stdlibs in the
next few years, we nip mallocarray() in the bud to allow an easy
transition to a system-provided version when the day comes.
1) Reorder local variables.
2) Cleanup error messages, use %zu for size_t.
3) combine putchar(' ') and fputs to substitute printf(" %s", s).
4) Fix usage().
5) argv-argc-usage-fix.
6) Add empty line before return.
A function used only in the OpenBSD-Kernel as of now, but it surely
provides a helpful interface when you just don't want to make sure
the incoming pointer to erealloc() is really NULL so it behaves
like malloc, making it a bit more safer.
Talking about *allocarray(): It's definitely a major step in code-
hardening. Especially as a system administrator, you should be
able to trust your core tools without having to worry about segfaults
like this, which can easily lead to privilege escalation.
How do the GNU coreutils handle this?
$ strings -n 4611686018427387903
strings: invalid minimum string length -1
$ strings -n 4611686018427387904
strings: invalid minimum string length 0
They silently overflow...
In comparison, sbase:
$ strings -n 4611686018427387903
mallocarray: out of memory
$ strings -n 4611686018427387904
mallocarray: out of memory
The first out of memory is actually a true OOM returned by malloc,
whereas the second one is a detected overflow, which is not marked
in a special way.
Now tell me which diagnostic error-messages are easier to understand.
Stateless and I stumbled upon this issue while discussing the
semantics of read, accepting a size_t but only being able to return
ssize_t, effectively lacking the ability to report successful
reads > SSIZE_MAX.
The discussion went along and we came to the topic of input-based
memory allocations. Basically, it was possible for the argument
to a memory-allocation-function to overflow, leading to a segfault
later.
The OpenBSD-guys came up with the ingenious reallocarray-function,
and I implemented it as ereallocarray, which automatically returns
on error.
Read more about it here[0].
A simple testcase is this (courtesy to stateless):
$ sbase-strings -n (2^(32|64) / 4)
This will segfault before this patch and properly return an OOM-
situation afterwards (thanks to the overflow-check in reallocarray).
[0]: http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/calloc.3
Allows dropping a local variable if the explicit PID is not needed
and it makes it clearer what happens.
Also, one should always strive for consistency for cases like these.
Quoting POSIX[0]:
"Care should be taken, also, to call _exit() rather than exit() if exec cannot be used, since
exit() flushes and closes standard I/O channels, thereby damaging the parent process' standard
I/O data structures. (Even with fork(), it is wrong to call exit(), since buffered data would
then be flushed twice.)"
[0]: http://pubs.opengroup.org/onlinepubs/009695399/functions/vfork.html
Similar to the chgrp(1)-audit:
1) Refactor manpage so it's actually fun to read
2) BUGFIX: Call (l)chown properly when the H-flag is specified
(only when depth > 0)
3) BUGFIX: Call (l)chown properly when the h-flag is specified
(only when depth = 0).
4) BUGFIX: Only recurse() in chgrp() when the initial chownf()
succeeds.
5) Style fixes, argv-basing.
6) Rename status to ret for consistency.
7) Add blank line before return.
1) Refactor manpage so it's actually fun to read.
2) BUGFIX: Call (l)chown properly when the H-flag is specified
(only when depth > 0).
3) BUGFIX: Call (l)chown properly when the h-flag is specified
(only when depth = 0).
4) BUGFIX: Only recurse() in chgrp() when the initial chownf()
succeeds.
5) Style fixes, argv-basing.
6) Rename status to ret for consistency.
7) Add blank line before return.
1) Update manpage with the num-syntax.
2) Use size_t for years and derivatives.
3) Use putchar instead of printf wherever possible.
4) Update usage().
5) Style changes.
1) Refactor manpage.
2) De-globalize local values.
3) update usage().
4) sort local variable declarations.
5) fix wrong argument in strtonum (3 -> 1).
6) argc-argv style, boolean style.
7) check bytes > 0 before accessing b.lines[i][bytes - 1]
relying on len only makes sense but let's not push it.
7) don't break on maxlen > (chars - 1) / 2. This didn't even
make sense.
8) _correctly_ calculate cols and rows in a readable way.
9) Rewrite loop over rows and cols in a readable way and
using putchar in a loop instead of printf-magic or fputs
where not necessary.
- Also fix a few typos, style and section order.
- Changed the text "800 characters per line" to "800 bytes per line" as col
doesn't seem to support UTF-8 right now.
