questionable code in nmbd/nmbd_packets.c?

[Cole, Timothy D.]

I've been trying to nail down some strange behavior with Samba over the
course of many months, and I've finally narrowed it down to the following
code in nmbd/nmbd_packet.c, lines 1080-1088:

      /* This is one occasion where we change a subnet that is
        given to us. If the packet was sent to WORKGROUP<1b> instead
        of WORKGROUP<1d> then it was unicast to us a domain master
        browser. Change subrec to unicast.
      */
      if(dgram->dest_name.name_type == 0x1b)
        subrec = unicast_subnet;
      
      process_get_backup_list_request(subrec, p, buf+1);

Why is this done?

Since unicast_subnet->myip is the IP of the WINS server, it results in the
source ip in the reply datagram being that of the WINS server (which is
wrong), and bogus data (servername<00> == WINS server IP) ends up in the
NetBIOS cache on the client as a result (also wrong).  This happens because
the subrec argument is subsequently passed to send_backup_list_response(),
where it is used to determine the source IP in the reply packet.

The subnet the original request was made on needs to be preserved.

IMO, process_get_backup_list_request() should take two subnet_record *
arguments, one being the subnet the original request was made on (which
would then be passed to send_backup_list_response()), the second being the
subnet for which the backup list is returned.

Alternatively, this check could be moved inside of
process_get_backup_list_request(), where the original subnet record can be
saved and passed on to send_backup_list_response().  I think that is the
cleaner approach.  If nobody has any objections, I'll have a patch for this
prepared shortly.