On Thu, 2019-11-14 at 07:33 -0600, Eric Blake wrote:
> On 11/14/19 4:04 AM, Maxim Levitsky wrote:
> > On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> > > As long as we limit NBD names to 256 bytes (the bare minimum permitted
> > > by the standard), stack-allocation works for parsing a name received
> > > from the client. But as mentioned in a comment, we eventually want to
> > > permit up to the 4k maximum of the NBD standard, which is too large
> > > for stack allocation; so switch everything in the server to use heap
> > > allocation. For now, there is no change in actually supported name
> > > length.
> >
> > I am just curios, why is this so?
> > I know that kernel uses 8K stacks due to historical limitation
> > of 1:1 physical memory mapping which creates fragmentation,
> > but in the userspace stacks shouldn't really be limited and grow on demand.
>
> Actually, 4k rather than 8k stack overflow guard pages are typical on
> some OS.
I was talking about the kernel stacks. These are limited to 8K with
no growing and it is a pain point there. Userspace stacks on the
other hand should be able to grow to an reasonable size.
> The problem with stack-allocating anything larger than the
> guard page size is that you can end up overshooting the guard page, and
> then the OS is unable to catch stack overflow in the normal manner of
> sending SIGSEGV. Also, when using coroutines, it is very common to have
> limited stack size in the first place, where large stack allocations can
> run into issues. So in general, it's a good rule of thumb to never
> stack-allocate something if it can be larger than 4k.
Doh! I know how the guard pages work, but never thought
about them in this way. I guess I don't after all.
Thanks for the explanation!
>
> > Some gcc security option limits this?
>
> Not by default, but you can compile with -Wframe-larger-than=4096 (or
> even smaller) to catch instances where stack allocation is likely to run
> into trouble.
>
>
> > > @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
> > > static int nbd_negotiate_handle_export_name(NBDClient *client, bool
> > > no_zeroes,
> > > Error **errp)
> > > {
> > > - char name[NBD_MAX_NAME_SIZE + 1];
> > > + g_autofree char *name;
> >
> > That is what patchew complained about I think.
>
> Yes, and I've already fixed the missing initializer.
>
> >
> > Isn't it wonderful how g_autofree fixes one issue
> > and introduces another. I mean 'name' isn't really
> > used here prior to allocation according to plain C,
> > but due to g_autofree, it can be now on any error
> > path. Nothing against g_autofree though, just noting this.
>
> Yes, and our documentation for g_auto* reminds that all such variables
> with automatic cleanup must have an initializer or be set prior to any
> exit path. I think I see why I didn't catch it beforehand - I'm
> compiling with --enable-debug, which passes CFLAGS=-g, while the
> compiler warning occurs when -O2 is in effect; but it is rather annoying
> that gcc doesn't catch the bug when not optimizing.
>
> >
> > Looks correct, but I might have missed something.
> >
> > Reviewed-by: Maxim Levitsky <mlevi...@redhat.com>
> >
>
> Thanks, and assuming that's with my initializer fix squashed in.
Of course.
Best regards,
Maxim Levitsky
