[SCM] Samba Shared Repository - branch master updated

[Richard Sharpe]

On Wed, Feb 19, 2014 at 5:13 PM, Jeremy Allison <jra at samba.org> wrote:
> On Wed, Feb 19, 2014 at 08:20:04PM +0100, Andreas Schneider wrote:
>> The branch, master has been updated
>
> Andreas and David,
>
>>        via  b8844fc clitar.c: check all allocations return value
>
> I *HATE* *HATE* *HATE* the idiom used here.
>
> Calling smb_panic on an allocation fail in the
> new tar code is manifestly the *WRONG* thing to
> do.
>
> +/* helper macro to die in case of NULL pointer */
> +#define PANIC_IF_NULL(x) \
> +    _panic_if_null(x, __FILE__ ":" STR2(__LINE__) " (" #x ") == NULL\n")
> +
> +/* prototype to silent gcc warning */
> +static inline void* _panic_if_null(void *p, const char *expr);
> +static inline void* _panic_if_null(void *p, const char *expr)
> +{
> +    if (!p) {
> +        smb_panic(expr);
> +    }
> +    return p;
> +}
> +
>
> Then later..
>
> +    TALLOC_CTX *ctx = PANIC_IF_NULL(talloc_new(NULL));
>
> and:
>
> +    fname = PANIC_IF_NULL(talloc_asprintf(ctx,
> +                                          "%s%s",
> +                                          client_get_cur_dir(),
> +                                          buf));
>      if (!fname) {
>          err = 1;
>          goto out;
>
> The above even left the (correct) handling of
> the NULL return alone !
>
> Remember on common setups this will call a
> panic action script which will leave the
> client hanging on a sleep 9999999 call and
> leave the caller with a hung command and no
> indication of what failed.
>
> Can you fix this please to do proper error
> checking of a NULL return and to bail out
> of the tar command with a correct error indication
> that the command failed ?
>
> This really needs fixing properly before this code
> can go anywhere near a production branch.

Personally, I would like to lobby for an extra parameter to smb_panic,
called dump_core or something.

Here is a situation where we don't need to dump core:

source3/smbd/server.c#6 (text)

@@ -640,6 +640,18 @@
                                         "because too many files are open\n"));
                                goto exit;
                        }
+
+                       /*
+                        * If we get NT_STATUS_OPEN_FAILED, there was
a problem with
+                        * re-initing the TDBs, so complain, but do
not dump core
+                        * as this can exacerbate the problems.
+                        */
+                       if (NT_STATUS_EQUAL(status, NT_STATUS_OPEN_FAILED)) {
+                               DEBUG(0, ("FATAL: child process cannot
initialize "
+                                         "because one or more TDBs
could not be reopened\n"));
+                               goto exit;
+                       }
+
                        if (lp_clustering() &&
                            NT_STATUS_EQUAL(status,
                            NT_STATUS_INTERNAL_DB_ERROR)) {

This happens after reinit_after_fork ...

When it happens because tdb_reopen_all failed, we should probably not
dump core, especially as that only exacerbates existing problems and
the core file tells us absolutely nothing new (the error in
tdb_reopen_all is long gone.)

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)