Re: af_alg: Fix state of stream after sendfile() succeeds

[Pádraig Brady]

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  <address@hidden>
> 
>       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