Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Maxim Levitsky]

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 <address@hidden>
> > 
> 
> Thanks, and assuming that's with my initializer fix squashed in.
Of course.

Best regards,
        Maxim Levitsky