[PATCH] Evaluate 'disable netbios' parameter

[Justin Stephenson]

Hi Noel, thank you for the review.

I made the requested changes, and converted the fprintf call to
DBG_ERR in [Patch 4/4]

Please see the updated patches.

Justin Stephenson
Red Hat

On Mon, Jan 7, 2019 at 1:54 PM Noel Power <nopower at suse.com> wrote:
>
> Hi Justin
>
> In [PATCH 3/4] s3:smbpasswd: Print debug message about Netbios
>
>      if (!NT_STATUS_IS_OK(result)) {
> +        if (NT_STATUS_EQUAL(result, NT_STATUS_NOT_SUPPORTED)) {
> +            rc = asprintf(&tmp_err, "NetBIOS support disabled, unable
> to connect");
> +            if (rc != -1) {
> +                *err_str = tmp_err;
> +            }
> +        }
> +
>          if (asprintf(err_str, "Unable to connect to SMB server on "
>               "machine %s. Error was : %s.\n",
>               remote_machine, nt_errstr(result))==-1) {
>
> The tmp_err seems unnecessary right? the intension is to populate
> err_str in anycase.
>
> rc is also I think redundant, might be better to use the existing
> pattern for instances of asprintf in the same method (which also in
> general also NULL-ify the err_str when asprintf fails)
>
> Wont err_str get overwritten with "Unable to connect to SMB..." here ? I
> guess you meant to have an 'else' matching the 'if
> (NT_STATUS_EQUAL(result, NT_STATUS_NOT_SUPPORTED)) {' and maybe have
> something like
>
> if (!NT_STATUS_IS_OK(result)) {
>     if (NT_STATUS_EQUAL(result, NT_STATUS_NOT_SUPPORTED)) {
>         if (asprintf(err_str, "Unable to connect to SMB server on "
>                       "machine %s. NetBIOS support disabled, "
>                       "unable to connect"\n",
>         remote_machine)==-1) {
>
>             ...
>                 }
>     } else {
>             if (asprintf(err_str, "Unable to connect to SMB server on "
>                       "machine %s. Error was : %s.\n",
>                       remote_machine, nt_errstr(result))==-1) {
>             ...
>         }
>     }
> ...
> }
>
> in [PATCH 4/4]  s3:utils:net: Print debug message about Netbios
>
> @@ -37,6 +37,10 @@ static time_t cli_servertime(const char *host,
>      status = cli_connect_nb(host, dest_ss, 0, 0x20, lp_netbios_name(),
>                  SMB_SIGNING_DEFAULT, 0, &cli);
>      if (!NT_STATUS_IS_OK(status)) {
> +        if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_SUPPORTED)) {
> +            DBG_ERR("NetBIOS support disabled, unable to connect\n");
> +        }
> +
>          fprintf(stderr, _("Can't contact server %s. Error %s\n"),
>              host, nt_errstr(status));
>
> Not sure what the feeling is about mixing DBG_ERR and
> fprintf(stderr,...) messages here, it looks odd to me, since it is
> DBG_ERR iiuc you will always get the output which to me would seem to
> say this should be part of the tool output and therefore be output like
> the rest e.g. via fprintf (but... that is just my opinion, others may
> differ)
>
> other than that this looks pretty good to me
>
> Noel
>
>
> On 04/01/2019 13:58, Justin Stephenson via samba-technical wrote:
> > Thank you for the review, I updated the patch with the suggestions
> > discussed here.
> >
> > Updated patch attached, please review.
> >
> > On Fri, Dec 21, 2018 at 1:11 PM Jeremy Allison <jra at samba.org> wrote:
> >> On Fri, Dec 21, 2018 at 09:04:09AM +0100, Stefan Metzmacher via samba-technical wrote:
> >>> Hi Justin,
> >>>
> >>>> The following patches evaluate the 'disable_netbios' option prior to making
> >>>> calls to cli_connect_nb() in several places, this bug was reported by a
> >>>> RHEL customer seeing outbound 'netbios-ssn' traffic on port 139 with
> >>>> disable_netbios set in smb.conf.
> >>>>
> >>>> Gitlab CI Passed, merge request below:
> >>>> https://gitlab.com/samba-team/samba/merge_requests/181
> >>>>
> >>>> Please review.
> >>> If we want to make sure we never generate traffic on port 139,
> >>> don't need to catch that in smbsock_connect_send()
> >>> if port 139 is explicitly passed. And don't try a fallback to that?
> >>>
> >>> What do others think?
> >> Yep, that would be the lowest level to catch it !
> >>
> >> We could return NT_STATUS_NOT_SUPPORTED on any
> >> 139 connect, and let the upper levels print
> >> the debugs as required.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jstephen_disable_netbios.patch
Type: text/x-patch
Size: 6445 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190108/2a47c4eb/jstephen_disable_netbios.bin>