On 24/06/18 17:58, Bruno Haible wrote:
> afalg_stream is documented as "Read from the current position to the
> end of STREAM." However, the current behaviour after sendfile() succeeds
> is that the stream is in an unusable state (except for fclose): It still
> has buffers allocated, but the underlying file descriptors is at a
> different position that what would be consistent with the buffers.
> You can see that by doing a getc() call afterwards.
>
> Should we change af_alg.h, md5.h, sha1.h etc. to all say "The only valid
> operation on STREAM afterwards is to close it."? I don't think this would
> be good: it would be a restriction that could too easily trigger bugs in
> the calling programs.
>
> So it's better to make sure STREAM is in a consistent state afterwards.
>
> This patch does it. In particular, it restores the fflush() call that Paul
> had eliminated on 2018-05-10.
>
>
> 2018-06-24 Bruno Haible <br...@clisp.org>
>
> af_alg: Fix state of stream after sendfile() succeeds.
> * lib/af_alg.c (afalg_stream): Invoke fflush and lseek, to ensure that
> the stream is correctly positioned afterwards.
> * modules/crypto/af_alg (Depends-on): Add fflush.
> * tests/test-digest.h (test_digest_on_files): Verify that after the
> operation the stream is positioned at end of file.
>
> diff --git a/lib/af_alg.c b/lib/af_alg.c
> index 555eb2b..2753ab9 100644
> --- a/lib/af_alg.c
> +++ b/lib/af_alg.c
> @@ -113,18 +113,47 @@ afalg_stream (FILE *stream, const char *alg,
> int fd = fileno (stream);
> int result;
> struct stat st;
> - off_t nseek = 0; /* Number of bytes to seek (backwards) in case of error.
> */
> off_t off = ftello (stream);
> if (0 <= off && fstat (fd, &st) == 0
> && (S_ISREG (st.st_mode) || S_TYPEISSHM (&st) || S_TYPEISTMO (&st))
> && off < st.st_size && st.st_size - off < SYS_BUFSIZE_MAX)
> {
> - off_t nbytes = st.st_size - off;
> - result = sendfile (ofd, fd, &off, nbytes) == nbytes ? 0 :
> -EAFNOSUPPORT;
> + /* Make sure the offset of fileno (stream) reflects how many bytes
> + have been read from stream before this function got invoked.
> + Note: fflush on an input stream after ungetc does not work as
> expected
> + on some platforms. Therefore this situation is not supported here.
> */
> + if (fflush (stream))
> + result = -EIO;
Might it be valid to return -EAFNOSUPPORT here instead,
so that the stream was processed with the non af_alg loop?
The fflush() makes sense to drop any stream buffers which
won't be used and will be invalid anyway
when we reset the file offset with lseek.
> + else
> + {
> + off_t nbytes = st.st_size - off;
> + if (sendfile (ofd, fd, &off, nbytes) == nbytes)
> + {
> + if (read (ofd, resblock, hashlen) == hashlen)
> + {
> + /* The input buffers of stream are no longer valid. */
> + if (lseek (fd, off, SEEK_SET) != (off_t)-1)
> + result = 0;
Should we be using fseek (as well) to set the stream position accordingly?
thanks,
Pádraig
