[PATCH] A few more around name lookup
Jeremy Allison
jra at samba.org
Tue Jan 23 23:56:27 UTC 2018
On Tue, Jan 23, 2018 at 08:33:41PM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
>
> Review appreciated!
LGTM. RB+ and pushed. Any chance in the future of making
parse_packet_talloc() the only external interface and
making parse_packet() static ?
Jeremy.
> --
> Besuchen Sie die verinice.XP 2018 in Berlin,
> Anwenderkonferenz für Informationssicherheit
> vom 21.-23.03.2018 im Sofitel Kurfürstendamm
> Info & Anmeldung hier: http://veriniceXP.org
>
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> From 3f9d67c97e3cabd309b5326ae061520cd37975c6 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 23 Jan 2018 14:39:21 +0100
> Subject: [PATCH 1/3] libnmb: Add "parse_packet_talloc"
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/nmblib.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
> source3/libsmb/nmblib.h | 5 ++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/source3/libsmb/nmblib.c b/source3/libsmb/nmblib.c
> index 8feb029..c262c28 100644
> --- a/source3/libsmb/nmblib.c
> +++ b/source3/libsmb/nmblib.c
> @@ -803,6 +803,77 @@ struct packet_struct *parse_packet(char *buf,int length,
> return p;
> }
>
> +static struct packet_struct *copy_packet_talloc(
> + TALLOC_CTX *mem_ctx, const struct packet_struct *src)
> +{
> + struct packet_struct *pkt;
> +
> + pkt = talloc_memdup(mem_ctx, src, sizeof(struct packet_struct));
> + if (pkt == NULL) {
> + return NULL;
> + }
> + pkt->locked = false;
> + pkt->recv_fd = -1;
> + pkt->send_fd = -1;
> +
> + if (src->packet_type == NMB_PACKET) {
> + const struct nmb_packet *nsrc = &src->packet.nmb;
> + struct nmb_packet *ndst = &pkt->packet.nmb;
> +
> + if (nsrc->answers != NULL) {
> + ndst->answers = talloc_memdup(
> + pkt, nsrc->answers,
> + sizeof(struct res_rec) * nsrc->header.ancount);
> + if (ndst->answers == NULL) {
> + goto fail;
> + }
> + }
> + if (nsrc->nsrecs != NULL) {
> + ndst->nsrecs = talloc_memdup(
> + pkt, nsrc->nsrecs,
> + sizeof(struct res_rec) * nsrc->header.nscount);
> + if (ndst->nsrecs == NULL) {
> + goto fail;
> + }
> + }
> + if (nsrc->additional != NULL) {
> + ndst->additional = talloc_memdup(
> + pkt, nsrc->additional,
> + sizeof(struct res_rec) * nsrc->header.arcount);
> + if (ndst->nsrecs == NULL) {
> + goto fail;
> + }
> + }
> + }
> +
> + return pkt;
> +
> + /*
> + * DGRAM packets have no substructures
> + */
> +
> +fail:
> + TALLOC_FREE(pkt);
> + return NULL;
> +}
> +
> +struct packet_struct *parse_packet_talloc(TALLOC_CTX *mem_ctx,
> + char *buf,int length,
> + enum packet_type packet_type,
> + struct in_addr ip,
> + int port)
> +{
> + struct packet_struct *pkt, *result;
> +
> + pkt = parse_packet(buf, length, packet_type, ip, port);
> + if (pkt == NULL) {
> + return NULL;
> + }
> + result = copy_packet_talloc(mem_ctx, pkt);
> + free_packet(pkt);
> + return result;
> +}
> +
> /*******************************************************************
> Read a packet from a socket and parse it, returning a packet ready
> to be used or put on the queue. This assumes a UDP socket.
> diff --git a/source3/libsmb/nmblib.h b/source3/libsmb/nmblib.h
> index 7e1e40c..1956ea7 100644
> --- a/source3/libsmb/nmblib.h
> +++ b/source3/libsmb/nmblib.h
> @@ -39,6 +39,11 @@ struct packet_struct *parse_packet(char *buf,int length,
> enum packet_type packet_type,
> struct in_addr ip,
> int port);
> +struct packet_struct *parse_packet_talloc(TALLOC_CTX *mem_ctx,
> + char *buf,int length,
> + enum packet_type packet_type,
> + struct in_addr ip,
> + int port);
> struct packet_struct *read_packet(int fd,enum packet_type packet_type);
> void make_nmb_name( struct nmb_name *n, const char *name, int type);
> bool nmb_name_equal(struct nmb_name *n1, struct nmb_name *n2);
> --
> 1.9.1
>
>
> From cf73144944f6db498c01a12de1770fee8360fcaa Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 16 Jan 2018 15:50:19 +0100
> Subject: [PATCH 2/3] libnmb: Make nb_packet_read_recv return a talloc'ed pkt
>
> This saves a few explicit destructors only doing free_packet()
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/clidgram.c | 3 +-
> source3/libsmb/namequery.c | 72 ++++++++-------------------------------------
> source3/libsmb/unexpected.c | 6 ++--
> source3/libsmb/unexpected.h | 2 +-
> 4 files changed, 19 insertions(+), 64 deletions(-)
>
> diff --git a/source3/libsmb/clidgram.c b/source3/libsmb/clidgram.c
> index 8f0dba3..4ae57a3 100644
> --- a/source3/libsmb/clidgram.c
> +++ b/source3/libsmb/clidgram.c
> @@ -397,7 +397,7 @@ static void nbt_getdc_got_response(struct tevent_req *subreq)
> NTSTATUS status;
> bool ret;
>
> - status = nb_packet_read_recv(subreq, &p);
> + status = nb_packet_read_recv(subreq, state, &p);
> TALLOC_FREE(subreq);
> if (tevent_req_nterror(req, status)) {
> return;
> @@ -406,7 +406,6 @@ static void nbt_getdc_got_response(struct tevent_req *subreq)
> ret = parse_getdc_response(p, state, state->domain_name,
> &state->nt_version, &state->dc_name,
> &state->samlogon_response);
> - free_packet(p);
> if (!ret) {
> tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
> return;
> diff --git a/source3/libsmb/namequery.c b/source3/libsmb/namequery.c
> index 6107e8f..b616a64 100644
> --- a/source3/libsmb/namequery.c
> +++ b/source3/libsmb/namequery.c
> @@ -309,7 +309,6 @@ struct sock_packet_read_state {
> struct packet_struct *packet;
> };
>
> -static int sock_packet_read_state_destructor(struct sock_packet_read_state *s);
> static void sock_packet_read_got_packet(struct tevent_req *subreq);
> static void sock_packet_read_got_socket(struct tevent_req *subreq);
>
> @@ -331,7 +330,6 @@ static struct tevent_req *sock_packet_read_send(
> if (req == NULL) {
> return NULL;
> }
> - talloc_set_destructor(state, sock_packet_read_state_destructor);
> state->ev = ev;
> state->reader = reader;
> state->sock = sock;
> @@ -359,15 +357,6 @@ static struct tevent_req *sock_packet_read_send(
> return req;
> }
>
> -static int sock_packet_read_state_destructor(struct sock_packet_read_state *s)
> -{
> - if (s->packet != NULL) {
> - free_packet(s->packet);
> - s->packet = NULL;
> - }
> - return 0;
> -}
> -
> static void sock_packet_read_got_packet(struct tevent_req *subreq)
> {
> struct tevent_req *req = tevent_req_callback_data(
> @@ -376,7 +365,7 @@ static void sock_packet_read_got_packet(struct tevent_req *subreq)
> req, struct sock_packet_read_state);
> NTSTATUS status;
>
> - status = nb_packet_read_recv(subreq, &state->packet);
> + status = nb_packet_read_recv(subreq, state, &state->packet);
>
> TALLOC_FREE(state->reader_req);
>
> @@ -398,8 +387,7 @@ static void sock_packet_read_got_packet(struct tevent_req *subreq)
> !state->validator(state->packet, state->private_data)) {
> DEBUG(10, ("validator failed\n"));
>
> - free_packet(state->packet);
> - state->packet = NULL;
> + TALLOC_FREE(state->packet);
>
> state->reader_req = nb_packet_read_send(state, state->ev,
> state->reader);
> @@ -460,8 +448,9 @@ static void sock_packet_read_got_socket(struct tevent_req *subreq)
> return;
> }
>
> - state->packet = parse_packet((char *)state->buf, received, state->type,
> - addr.sin.sin_addr, addr.sin.sin_port);
> + state->packet = parse_packet_talloc(
> + state, (char *)state->buf, received, state->type,
> + addr.sin.sin_addr, addr.sin.sin_port);
> if (state->packet == NULL) {
> DEBUG(10, ("parse_packet failed\n"));
> goto retry;
> @@ -483,10 +472,7 @@ static void sock_packet_read_got_socket(struct tevent_req *subreq)
> return;
>
> retry:
> - if (state->packet != NULL) {
> - free_packet(state->packet);
> - state->packet = NULL;
> - }
> + TALLOC_FREE(state->packet);
> TALLOC_FREE(state->buf);
> TALLOC_FREE(state->addr);
>
> @@ -499,6 +485,7 @@ retry:
> }
>
> static NTSTATUS sock_packet_read_recv(struct tevent_req *req,
> + TALLOC_CTX *mem_ctx,
> struct packet_struct **ppacket)
> {
> struct sock_packet_read_state *state = tevent_req_data(
> @@ -508,8 +495,7 @@ static NTSTATUS sock_packet_read_recv(struct tevent_req *req,
> if (tevent_req_is_nterror(req, &status)) {
> return status;
> }
> - *ppacket = state->packet;
> - state->packet = NULL;
> + *ppacket = talloc_move(mem_ctx, &state->packet);
> return NT_STATUS_OK;
> }
>
> @@ -532,7 +518,6 @@ struct nb_trans_state {
> struct packet_struct *packet;
> };
>
> -static int nb_trans_state_destructor(struct nb_trans_state *s);
> static void nb_trans_got_reader(struct tevent_req *subreq);
> static void nb_trans_done(struct tevent_req *subreq);
> static void nb_trans_sent(struct tevent_req *subreq);
> @@ -564,7 +549,6 @@ static struct tevent_req *nb_trans_send(
> if (req == NULL) {
> return NULL;
> }
> - talloc_set_destructor(state, nb_trans_state_destructor);
> state->ev = ev;
> state->buf = buf;
> state->buflen = buflen;
> @@ -604,15 +588,6 @@ static struct tevent_req *nb_trans_send(
> return req;
> }
>
> -static int nb_trans_state_destructor(struct nb_trans_state *s)
> -{
> - if (s->packet != NULL) {
> - free_packet(s->packet);
> - s->packet = NULL;
> - }
> - return 0;
> -}
> -
> static void nb_trans_got_reader(struct tevent_req *subreq)
> {
> struct tevent_req *req = tevent_req_callback_data(
> @@ -704,7 +679,7 @@ static void nb_trans_done(struct tevent_req *subreq)
> req, struct nb_trans_state);
> NTSTATUS status;
>
> - status = sock_packet_read_recv(subreq, &state->packet);
> + status = sock_packet_read_recv(subreq, state, &state->packet);
> TALLOC_FREE(subreq);
> if (tevent_req_nterror(req, status)) {
> return;
> @@ -712,7 +687,7 @@ static void nb_trans_done(struct tevent_req *subreq)
> tevent_req_done(req);
> }
>
> -static NTSTATUS nb_trans_recv(struct tevent_req *req,
> +static NTSTATUS nb_trans_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
> struct packet_struct **ppacket)
> {
> struct nb_trans_state *state = tevent_req_data(
> @@ -722,8 +697,7 @@ static NTSTATUS nb_trans_recv(struct tevent_req *req,
> if (tevent_req_is_nterror(req, &status)) {
> return status;
> }
> - *ppacket = state->packet;
> - state->packet = NULL;
> + *ppacket = talloc_move(mem_ctx, &state->packet);
> return NT_STATUS_OK;
> }
>
> @@ -740,8 +714,6 @@ struct node_status_query_state {
> struct packet_struct *packet;
> };
>
> -static int node_status_query_state_destructor(
> - struct node_status_query_state *s);
> static bool node_status_query_validator(struct packet_struct *p,
> void *private_data);
> static void node_status_query_done(struct tevent_req *subreq);
> @@ -762,7 +734,6 @@ struct tevent_req *node_status_query_send(TALLOC_CTX *mem_ctx,
> if (req == NULL) {
> return NULL;
> }
> - talloc_set_destructor(state, node_status_query_state_destructor);
>
> if (addr->ss_family != AF_INET) {
> /* Can't do node status to IPv6 */
> @@ -837,16 +808,6 @@ static bool node_status_query_validator(struct packet_struct *p,
> return true;
> }
>
> -static int node_status_query_state_destructor(
> - struct node_status_query_state *s)
> -{
> - if (s->packet != NULL) {
> - free_packet(s->packet);
> - s->packet = NULL;
> - }
> - return 0;
> -}
> -
> static void node_status_query_done(struct tevent_req *subreq)
> {
> struct tevent_req *req = tevent_req_callback_data(
> @@ -855,7 +816,7 @@ static void node_status_query_done(struct tevent_req *subreq)
> req, struct node_status_query_state);
> NTSTATUS status;
>
> - status = nb_trans_recv(subreq, &state->packet);
> + status = nb_trans_recv(subreq, state, &state->packet);
> TALLOC_FREE(subreq);
> if (tevent_req_nterror(req, status)) {
> return;
> @@ -1501,7 +1462,7 @@ static void name_query_done(struct tevent_req *subreq)
> NTSTATUS status;
> struct packet_struct *p = NULL;
>
> - status = nb_trans_recv(subreq, &p);
> + status = nb_trans_recv(subreq, state, &p);
> TALLOC_FREE(subreq);
> if (tevent_req_nterror(req, status)) {
> return;
> @@ -1510,13 +1471,6 @@ static void name_query_done(struct tevent_req *subreq)
> tevent_req_nterror(req, state->validate_error);
> return;
> }
> - if (p != NULL) {
> - /*
> - * Free the packet here, we've collected the response in the
> - * validator
> - */
> - free_packet(p);
> - }
> tevent_req_done(req);
> }
>
> diff --git a/source3/libsmb/unexpected.c b/source3/libsmb/unexpected.c
> index 16d1f67..ac6c1cf 100644
> --- a/source3/libsmb/unexpected.c
> +++ b/source3/libsmb/unexpected.c
> @@ -715,7 +715,7 @@ static void nb_packet_read_done(struct tevent_req *subreq)
> tevent_req_done(req);
> }
>
> -NTSTATUS nb_packet_read_recv(struct tevent_req *req,
> +NTSTATUS nb_packet_read_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
> struct packet_struct **ppacket)
> {
> struct nb_packet_read_state *state = tevent_req_data(
> @@ -731,7 +731,8 @@ NTSTATUS nb_packet_read_recv(struct tevent_req *req,
>
> memcpy(&hdr, state->buf, sizeof(hdr));
>
> - packet = parse_packet(
> + packet = parse_packet_talloc(
> + mem_ctx,
> (char *)state->buf + sizeof(struct nb_packet_client_header),
> state->buflen - sizeof(struct nb_packet_client_header),
> state->hdr.type, state->hdr.ip, state->hdr.port);
> @@ -739,6 +740,7 @@ NTSTATUS nb_packet_read_recv(struct tevent_req *req,
> tevent_req_received(req);
> return NT_STATUS_INVALID_NETWORK_RESPONSE;
> }
> +
> *ppacket = packet;
> tevent_req_received(req);
> return NT_STATUS_OK;
> diff --git a/source3/libsmb/unexpected.h b/source3/libsmb/unexpected.h
> index a40a507..270976b 100644
> --- a/source3/libsmb/unexpected.h
> +++ b/source3/libsmb/unexpected.h
> @@ -43,7 +43,7 @@ NTSTATUS nb_packet_reader_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
> struct tevent_req *nb_packet_read_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct nb_packet_reader *reader);
> -NTSTATUS nb_packet_read_recv(struct tevent_req *req,
> +NTSTATUS nb_packet_read_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx,
> struct packet_struct **ppacket);
>
> #endif
> --
> 1.9.1
>
>
> From 51b5e009c87b3988a2a07346efae7a81a4ebb434 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Tue, 16 Jan 2018 16:21:08 +0100
> Subject: [PATCH 3/3] libnmb: Move "read_packet" to nmbd
>
> It's only used there
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/libsmb/nmblib.c | 35 -----------------------------------
> source3/libsmb/nmblib.h | 1 -
> source3/nmbd/nmbd_packets.c | 35 +++++++++++++++++++++++++++++++++++
> 3 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/source3/libsmb/nmblib.c b/source3/libsmb/nmblib.c
> index c262c28..bac4340 100644
> --- a/source3/libsmb/nmblib.c
> +++ b/source3/libsmb/nmblib.c
> @@ -875,41 +875,6 @@ struct packet_struct *parse_packet_talloc(TALLOC_CTX *mem_ctx,
> }
>
> /*******************************************************************
> - Read a packet from a socket and parse it, returning a packet ready
> - to be used or put on the queue. This assumes a UDP socket.
> -******************************************************************/
> -
> -struct packet_struct *read_packet(int fd,enum packet_type packet_type)
> -{
> - struct packet_struct *packet;
> - struct sockaddr_storage sa;
> - struct sockaddr_in *si = (struct sockaddr_in *)&sa;
> - char buf[MAX_DGRAM_SIZE];
> - int length;
> -
> - length = read_udp_v4_socket(fd,buf,sizeof(buf),&sa);
> - if (length < MIN_DGRAM_SIZE || sa.ss_family != AF_INET) {
> - return NULL;
> - }
> -
> - packet = parse_packet(buf,
> - length,
> - packet_type,
> - si->sin_addr,
> - ntohs(si->sin_port));
> - if (!packet)
> - return NULL;
> -
> - packet->recv_fd = fd;
> - packet->send_fd = -1;
> -
> - DEBUG(5,("Received a packet of len %d from (%s) port %d\n",
> - length, inet_ntoa(packet->ip), packet->port ) );
> -
> - return(packet);
> -}
> -
> -/*******************************************************************
> Send a udp packet on a already open socket.
> ******************************************************************/
>
> diff --git a/source3/libsmb/nmblib.h b/source3/libsmb/nmblib.h
> index 1956ea7..a0624ed 100644
> --- a/source3/libsmb/nmblib.h
> +++ b/source3/libsmb/nmblib.h
> @@ -44,7 +44,6 @@ struct packet_struct *parse_packet_talloc(TALLOC_CTX *mem_ctx,
> enum packet_type packet_type,
> struct in_addr ip,
> int port);
> -struct packet_struct *read_packet(int fd,enum packet_type packet_type);
> void make_nmb_name( struct nmb_name *n, const char *name, int type);
> bool nmb_name_equal(struct nmb_name *n1, struct nmb_name *n2);
> int build_packet(char *buf, size_t buflen, struct packet_struct *p);
> diff --git a/source3/nmbd/nmbd_packets.c b/source3/nmbd/nmbd_packets.c
> index 2b7cc82..bbcb958 100644
> --- a/source3/nmbd/nmbd_packets.c
> +++ b/source3/nmbd/nmbd_packets.c
> @@ -1879,6 +1879,41 @@ static void nmbd_fd_handler(struct tevent_context *ev,
> attr->triggered = true;
> }
>
> +/*******************************************************************
> + Read a packet from a socket and parse it, returning a packet ready
> + to be used or put on the queue. This assumes a UDP socket.
> +******************************************************************/
> +
> +static struct packet_struct *read_packet(int fd,enum packet_type packet_type)
> +{
> + struct packet_struct *packet;
> + struct sockaddr_storage sa;
> + struct sockaddr_in *si = (struct sockaddr_in *)&sa;
> + char buf[MAX_DGRAM_SIZE];
> + int length;
> +
> + length = read_udp_v4_socket(fd,buf,sizeof(buf),&sa);
> + if (length < MIN_DGRAM_SIZE || sa.ss_family != AF_INET) {
> + return NULL;
> + }
> +
> + packet = parse_packet(buf,
> + length,
> + packet_type,
> + si->sin_addr,
> + ntohs(si->sin_port));
> + if (!packet)
> + return NULL;
> +
> + packet->recv_fd = fd;
> + packet->send_fd = -1;
> +
> + DEBUG(5,("Received a packet of len %d from (%s) port %d\n",
> + length, inet_ntoa(packet->ip), packet->port ) );
> +
> + return(packet);
> +}
> +
> /****************************************************************************
> Listens for NMB or DGRAM packets, and queues them.
> return True if the socket is dead
> --
> 1.9.1
>
More information about the samba-technical
mailing list