Re: [PATCH v2 1/5] mips: Do not malloc on getdents64 fallback

[Florian Weimer <fweimer at redhat dot com>]

Sorry, I missed that despite all the 64s in the patch, this code is also
used on 32-bit architectures.  The review below should take this new (to
me) piece of information into account.

* Adhemerval Zanella:

> +  static bool getdents64_supportted = true;
> +  if (atomic_load_relaxed (&getdents64_supportted))

Our atomics do not support bool, only 4 bytes and 8 bytes (if
__HAVE_64B_ATOMICS) is defined.  See __atomic_check_size.

Probably it will work if it compiles, but I haven't checked that.

>    /* Unfortunately getdents64 was only wire-up for MIPS n64 on Linux 3.10.
> +     If the syscall is not available it need to fallback to the non-LFS one.
> +     Also to avoid an unbounded allocation through VLA/alloca or malloc (which
> +     would make the syscall non async-signal-safe) it uses a limited buffer.
> +     This is sub-optimal for large NBYTES, however this is a fallback
> +     mechanism to emulate a syscall that kernel should provide.   */
>  
> +  enum { KBUF_SIZE = 1024 };

The choice of size needs a comment.  I think the largest possible
practical length of the d_name member are 255 Unicode characters in the
BMP, in UTF-8 encoding, so d_name is 766 bytes long, plus 10 bytes from
the header, for 776 bytes total.  (NAME_MAX is not a constant on Linux
in reality.)

>    struct kernel_dirent
> +  {
> +    unsigned long d_ino;
> +    unsigned long d_off;
> +    unsigned short int d_reclen;
> +    char d_name[1];
> +  } kbuf[KBUF_SIZE / sizeof (struct kernel_dirent)];
> +  size_t kbuf_size = nbytes < KBUF_SIZE ? nbytes : KBUF_SIZE;

I would define kbuf as a char array, and perhaps leave out the d_name
member in struct kernel_dirent.  You can copy out the struct
kernel_dirent using memcpy, which GCC should optimize away.

Ideally, we would perform the conversion in-line, with a forward scan to
make the d_reclen members point backwards, followed by a backwards scan
to move everything in place.  This would reduce stack usage quite
significantly and avoid a hard restriction on d_name length.

>    struct dirent64 *dp = (struct dirent64 *) buf;
> +
> +  size_t nb = 0;
> +  off64_t last_offset = -1;
> +
> +  ssize_t r;
> +  while ((r = INLINE_SYSCALL_CALL (getdents, fd, kbuf, kbuf_size)) > 0)
>      {

Sorry, I don't see how the outer loop is exited.  I think we should
remove it because it does not seem necessary.

> +      struct kernel_dirent *skdp, *kdp;
> +      skdp = kdp = kbuf;
> +
> +      while ((char *) kdp < (char *) skdp + r)
> +	{
> +	  const size_t alignment = _Alignof (struct dirent64);
> +	  size_t new_reclen = ((kdp->d_reclen + alignment - 1)
> +			      & ~(alignment - 1));

I think this is the roundup macro.  If you use that, I think you don't
need the alignment variable.

Is the length really correct, though?  I'd expect it to grow by the
additional size of the d_ino and d_off members.  I think it would be
best recompute it from scratch, using the actual length of d_name.

> +	  if (nb + new_reclen > nbytes)
>  	    {
> +		/* The new entry will overflow the input buffer, rewind to
> +		   last obtained entry and return.  */
> +	       __lseek64 (fd, last_offset, SEEK_SET);

I don't think last_offset is guaranteed to have been set with a proper
offset at this point.  Given that d_name is essentially of unbounded
length, even expanding the first entry can cause failure.

Maybe it's possible to avoid this corner case by limiting the amount of
data being read so that we know that the application-supplied buffer is
always large enough for any possible expansion.  I think the worse-case
growth is for lengths 5 to 8, from 20 bytes to 32 bytes.  So perhaps we
should divide the buffer size by 1.6 and use that?

> +	       goto out;
>  	    }
> +	  nb += new_reclen;
>  
> +	  dp->d_ino = kdp->d_ino;
> +	  dp->d_off = last_offset = kdp->d_off;
> +	  dp->d_reclen = new_reclen;
> +	  dp->d_type = *((char *) kdp + kdp->d_reclen - 1);

I think instead of reading through kdp, you should use char *s and
memcpy, to avoid the aliasing violation, as discussed above.  Likewise
for writing to dp.

> +	  memcpy (dp->d_name, kdp->d_name,
> +		  kdp->d_reclen - offsetof (struct kernel_dirent, d_name));

See above, I have concerns about the length.

Thanks,
Florian