[PATCH] Evaluate 'disable netbios' parameter

[Noel Power]

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.