Hi Joao,
Joao Matos <[email protected]> writes:
> As Paul suggested, our patch and its test are attached. Let us know if
> you would like any changes.
>
> In any case, thanks for taking the report into account.
Nice work! Those patches look pretty good to me. Some comments below
inline, but no need to send a v2 patch. There are just some tiny changes
that I made locally, and I figured they were worth sharing for your
reference.
> + /* No O_TRUNC: the output may be the input, not read to the end yet.
> + Truncate only once it is known to be another file. */
> + int fd = open (output_filename, O_WRONLY | O_CREAT | O_BINARY,
> + MODE_RW_UGO);
I don't think O_BINARY is needed here, because 'csplit' operates on text
files.
> + output_stream = 0 <= fd ? fdopen (fd, "w") : NULL;
That said, I think this would associate it with a text stream. I'm not
100% sure about that, so I'll remove O_BINARY just to be safe.
If you run 'make syntax-check' you will see the following error:
0.02 prohibit_stddef_without_use
prohibit_stdio--_without_use
src/csplit.c
maint.mk: the above files include stdio--.h but don't use it
make: *** [maint.mk:816: sc_prohibit_stdio--_without_use] Error 1
That tells us we can remove the header since we no longer use fopen. We
replace fopen with a macro pointing to fopen_safer, which will ensure
the file stream is not associated with STDIN_FILENO, STDOUT_FILENO, or
STDERR_FILENO. That can occur in a case like this:
$ seq 2 | strace --quiet=all -e trace='/fcntl|open' \
-P xx00 csplit - '/[0-9]$/' '{*}' 1<&-
openat(AT_FDCWD, "xx00", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 1
fcntl(1, F_DUPFD, 3) = 3
fcntl(3, F_GETFL) = 0x8001 (flags
O_WRONLY|O_LARGEFILE)
csplit: write error: Bad file descriptor
In the above output, we see "xx00" is opened as 1 (STDOUT_FILENO), but
we use a call to fcntl (1, F_DUPFD, 3) to open at file descriptor 3
(STDERR_FILENO + 1).
The 'make syntax-check' rules include some general style checks, e.g.,
removing that now-unnecessary header. Also, some checks for gotchas,
e.g., non-portable shell features or things that would mess up the
release tarball, e.g., invalid patches to Gnulib.
Anyways, with this tiny change I applied locally it succeeds:
diff --git a/src/csplit.c b/src/csplit.c
index f1d23ace0..c7b5718ee 100644
--- a/src/csplit.c
+++ b/src/csplit.c
@@ -31,7 +31,6 @@
#include "fd-reopen.h"
#include "quote.h"
#include "safe-read.h"
-#include "stdio--.h"
#include "term-sig.h"
#include "xdectoint.h"
#include "xstrtol.h"
The tests look pretty look pretty complete from my quick look over them.
Since those changes are quite large, especially the tests, in terms of
lines of code, would you be willing to sign a copyright assignment? If
so, I can email you privately with the steps.
Thanks,
Collin
