[PATCHES] More warning cleanup / minor fixes
John Lauro <[email protected]> Tue, 22 Oct 2013 15:04:14 -0400 (EDT)
| Newsgroups | gmane.network.poptop |
|---|---|
| Message-ID | <[email protected]> |
This is available from git://git.code.sf.net/u/johnalauro/poptop (branch master, in several small commits for easier review or commit) This clears up all the compiler warnings for me. I would be interested in knowing if any other environment generates any warnings... It uses alloca now to allocate a packet structure for better type checking instead of a fixed length unsigned char array, which as far as I know is available everywhere, but I only tested on one version of Linux. It would be good to know for any other platforms to test if it breaks compile and needs to be done differently. Changelog and NEWS might be better formatted/written. Feel free to redo... Wasn't sure how picky or any style guideline is for those files... Changelog also includes some summaries from my previous round. It's been running half a day on my server, so far so good. (Only half a dozen or so concurrent connections). Just a note, I think at least the experimental version at http://poptop.sourceforge.net/ should reference the git repo if not be an updated experimental release. As far as I know, this is the only open source pptp server. Please correct me if I am wrong, but I think the other pptp options are client only? diff --git a/ChangeLog b/ChangeLog index 79a2b65..5f2d819 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,45 @@ +Mon Oct 21 20:50:07 2013 John Lauro <[email protected]> + Don't leak memory from duplicate packet + pqueue.c + + Don't leak handle on connect fail and detect error on socket creation instead later + pptpgre.c + pptpmanager.c + + Code cleanup - waring fix + bcrelay.c + + Clean up warning dereferecing type-punned pointer from mixing struct and unsigned char + ctrlpacket.c + ctrlpacket.h + pptpctrl.c + + Fix several warnings in newer compilers, caused by format and type + discrepancies. + Author: [email protected] + Reviewed-by: James Cameron <[email protected]> + ctrlpacket.c + pptpgre.c + + Description: Use unsigned types to properly handle negative error codes + Author: [email protected] + Reviewed-by: James Cameron <[email protected]> + ctrlpacket.c + ctrlpacket.h + + Add missing change from previous patch + bcrelay.c + Code cleanup (potential off by one error, bcrelay) + Switch a copy function to strncpy, as original had a potential off by + one error as the size check didn't account for trailing 0 to be added + in next line. + bcrelay.c + + Fix several warnings in newer compilers, caused by format and type + discrepancies, and comparing with == directly to "", as "" is not + guaranteed to always be the same when redefined. + bcrelay.c + Thu Feb 7 11:51:46 2013 James Cameron <[email protected]> * plugins/pptpd-logwtmp.c: use pppd.h provided by ppp package diff --git a/NEWS b/NEWS index c736f68..675e434 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,5 @@ +- don't leak memory on duplicate packet [Lauro] +- Clean up several compiler warnings [Lauro] - add support for VRFs through libvrf [Lamparter] - fix implementation of IDLE_WAIT [Douglass] - fix compilation with uclibc with legacy support disabled [Hiramoto] diff --git a/bcrelay.c b/bcrelay.c index b0c472b..e091850 100644 --- a/bcrelay.c +++ b/bcrelay.c @@ -426,7 +426,7 @@ static void mainloop(int argc, char **argv) struct iflist *iflist = NULL; // Initialised after the 1st packet struct sockaddr_ll sa; struct packet *ipp_p; - char *udppdu; // FIXME: warning: pointer targets in assignment differ in signedness + unsigned char *udppdu; fd_set sock_set; struct timeval time_2_wait; static struct ifsnr old_ifsnr[MAXIF+1]; // Old iflist to socket fd's mapping list diff --git a/ctrlpacket.c b/ctrlpacket.c index 63c6d5e..091cdd4 100644 --- a/ctrlpacket.c +++ b/ctrlpacket.c @@ -37,11 +37,11 @@ /* Local function prototypes */ static ssize_t read_pptp_header(int clientFd, unsigned char *packet, int *ctrl_message_type); -static void deal_start_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); -static void deal_stop_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); -static void deal_out_call(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); -static void deal_echo(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); -static void deal_call_clr(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); +static void deal_start_ctrl_conn(void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +static void deal_stop_ctrl_conn(void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +static void deal_out_call(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +static void deal_echo(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +static void deal_call_clr(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); static void deal_set_link_info(unsigned char *packet); static u_int16_t getcall(); static u_int16_t freecall(); @@ -66,7 +66,7 @@ static int make_out_call_rqst(unsigned char *rply_packet, ssize_t * rply_size); * -1 on retryable error. * 0 on error to abort on. */ -int read_pptp_packet(int clientFd, unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size) +int read_pptp_packet(int clientFd, void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { ssize_t bytes_read; @@ -133,7 +133,7 @@ int read_pptp_packet(int clientFd, unsigned char *packet, unsigned char *rply_pa * retn: Number of bytes written on success. * -1 on write failure. */ -ssize_t send_pptp_packet(int clientFd, unsigned char *packet, size_t packet_size) +ssize_t send_pptp_packet(int clientFd, void *packet, size_t packet_size) { ssize_t bytes_written; @@ -362,7 +362,7 @@ ssize_t read_pptp_header(int clientFd, unsigned char *packet, int *pptp_ctrl_typ * rply_packet (OUT) - suitable reply to the 'packet' we got. * rply_size (OUT) - size of the reply packet */ -void deal_start_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size) +void deal_start_ctrl_conn(void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { struct pptp_start_ctrl_conn_rqst *start_ctrl_conn_rqst; struct pptp_start_ctrl_conn_rply start_ctrl_conn_rply; @@ -391,7 +391,7 @@ void deal_start_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssi * This method response to a STOP-CONTROL-CONNECTION-REQUEST with a * STOP-CONTROL-CONNECTION-REPLY. */ -void deal_stop_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size) +void deal_stop_ctrl_conn(void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { struct pptp_stop_ctrl_conn_rply stop_ctrl_conn_rply; @@ -416,7 +416,7 @@ void deal_stop_ctrl_conn(unsigned char *packet, unsigned char *rply_packet, ssiz * rply_size (OUT) - size of the reply packet * */ -void deal_out_call(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size) +void deal_out_call(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { u_int16_t pac_call_id; struct pptp_out_call_rqst *out_call_rqst; @@ -468,7 +468,7 @@ void deal_out_call(unsigned char *packet, unsigned char *rply_packet, ssize_t * * rply_size (OUT) - size of the reply packet * */ -void deal_echo(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size) +void deal_echo(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { struct pptp_echo_rqst *echo_rqst; struct pptp_echo_rply echo_rply; @@ -497,7 +497,7 @@ void deal_echo(unsigned char *packet, unsigned char *rply_packet, ssize_t * rply * rply_size (OUT) - size of the reply packet * */ -void deal_call_clr(unsigned char *packet, unsigned char *rply_packet, ssize_t *rply_size) +void deal_call_clr(unsigned char *packet, struct pptp_out_call_rply *rply_packet, ssize_t *rply_size) { struct pptp_call_disconn_ntfy call_disconn_ntfy; u_int16_t pac_call_id; @@ -554,7 +554,7 @@ void deal_set_link_info(unsigned char *packet) syslog(LOG_DEBUG, "CTRL: Got a SET LINK INFO packet with standard ACCMs"); } -void make_echo_req_packet(unsigned char *rply_packet, ssize_t * rply_size, u_int32_t echo_id) +void make_echo_req_packet(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size, u_int32_t echo_id) { struct pptp_echo_rqst echo_packet; @@ -564,7 +564,7 @@ void make_echo_req_packet(unsigned char *rply_packet, ssize_t * rply_size, u_int DEBUG_PACKET("ECHO REQ"); } -void make_stop_ctrl_req(unsigned char *rply_packet, ssize_t * rply_size) +void make_stop_ctrl_req(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { struct pptp_stop_ctrl_conn_rqst stop_ctrl; @@ -576,7 +576,7 @@ void make_stop_ctrl_req(unsigned char *rply_packet, ssize_t * rply_size) DEBUG_PACKET("STOP CTRL REQ"); } -void make_call_admin_shutdown(unsigned char *rply_packet, ssize_t * rply_size) +void make_call_admin_shutdown(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size) { struct pptp_call_disconn_ntfy call_disconn_ntfy; u_int16_t pac_call_id; diff --git a/ctrlpacket.h b/ctrlpacket.h index 26449ec..cc078a4 100644 --- a/ctrlpacket.h +++ b/ctrlpacket.h @@ -9,10 +9,10 @@ #ifndef _PPTPD_CTRLPACKET_H #define _PPTPD_CTRLPACKET_H -int read_pptp_packet(int clientFd, unsigned char *packet, unsigned char *rply_packet, ssize_t * rply_size); -ssize_t send_pptp_packet(int clientFd, unsigned char *packet, size_t packet_size); -void make_echo_req_packet(unsigned char *rply_packet, ssize_t * rply_size, u_int32_t echo_id); -void make_call_admin_shutdown(unsigned char *rply_packet, ssize_t * rply_size); -void make_stop_ctrl_req(unsigned char *rply_packet, ssize_t * rply_size); +int read_pptp_packet(int clientFd, void *packet, struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +ssize_t send_pptp_packet(int clientFd, void *packet, size_t packet_size); +void make_echo_req_packet(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size, u_int32_t echo_id); +void make_call_admin_shutdown(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); +void make_stop_ctrl_req(struct pptp_out_call_rply *rply_packet, ssize_t * rply_size); #endif /* !_PPTPD_CTRLPACKET_H */ diff --git a/pptpctrl.c b/pptpctrl.c index d347bc5..f470b4c 100644 --- a/pptpctrl.c +++ b/pptpctrl.c @@ -259,8 +259,8 @@ static void pptp_handle_ctrl_connection(char **pppaddrs, struct in_addr *inetadd int gre_fd = -1; /* Network file descriptor */ int sig_fd = sigpipe_fd(); /* Signal pipe descriptor */ - unsigned char packet[PPTP_MAX_CTRL_PCKT_SIZE]; - unsigned char rply_packet[PPTP_MAX_CTRL_PCKT_SIZE]; + struct pptp_echo_rply *packet = alloca(PPTP_MAX_CTRL_PCKT_SIZE); + struct pptp_out_call_rply *rply_packet = alloca(PPTP_MAX_CTRL_PCKT_SIZE); for (;;) { @@ -336,7 +336,7 @@ static void pptp_handle_ctrl_connection(char **pppaddrs, struct in_addr *inetadd if (FD_ISSET(clientSocket, &fds)) { time(&last_time); send_packet = TRUE; - switch (read_pptp_packet(clientSocket, packet, rply_packet, &rply_size)) { + switch (read_pptp_packet(clientSocket, (unsigned char *) packet, rply_packet, &rply_size)) { case 0: syslog(LOG_ERR, "CTRL: CTRL read failed"); goto leave_drop_call; @@ -373,8 +373,8 @@ static void pptp_handle_ctrl_connection(char **pppaddrs, struct in_addr *inetadd case OUT_CALL_RQST: /* for killing off the link later (ugly) */ - NOTE_VALUE(PAC, call_id_pair, ((struct pptp_out_call_rply *) (rply_packet))->call_id); - NOTE_VALUE(PNS, call_id_pair, ((struct pptp_out_call_rply *) (rply_packet))->call_id_peer); + NOTE_VALUE(PAC, call_id_pair, rply_packet->call_id); + NOTE_VALUE(PNS, call_id_pair, rply_packet->call_id_peer); if (gre_fd != -1 || pty_fd != -1) { syslog(LOG_WARNING, "CTRL: Request to open call when call is already open, closing"); if (gre_fd != -1) { @@ -392,8 +392,8 @@ static void pptp_handle_ctrl_connection(char **pppaddrs, struct in_addr *inetadd my_setproctitle(gargc, gargv, "pptpd [%s:%04X - %04X]", inet_ntoa(inetaddrs[1]), - ntohs(((struct pptp_out_call_rply *) (rply_packet))->call_id_peer), - ntohs(((struct pptp_out_call_rply *) (rply_packet))->call_id)); + ntohs(rply_packet->call_id_peer), + ntohs(rply_packet->call_id)); /* start the call, by launching pppd */ syslog(LOG_INFO, "CTRL: Starting call (launching pppd, opening GRE)"); pty_fd = startCall(pppaddrs, inetaddrs); @@ -403,7 +403,7 @@ static void pptp_handle_ctrl_connection(char **pppaddrs, struct in_addr *inetadd break; case ECHO_RPLY: - if (echo_wait == TRUE && ((struct pptp_echo_rply *) (packet))->identifier == echo_count) + if (echo_wait == TRUE && packet->identifier == echo_count) echo_wait = FALSE; else syslog(LOG_WARNING, "CTRL: Unexpected ECHO REPLY packet"); @@ -489,7 +489,7 @@ static void bail(int sigraised) fd_set connSet; /* fd_set for select() */ struct timeval tv; /* time to wait for reply */ unsigned char packet[PPTP_MAX_CTRL_PCKT_SIZE]; - unsigned char rply_packet[PPTP_MAX_CTRL_PCKT_SIZE]; + struct pptp_out_call_rply *rply_packet = alloca(PPTP_MAX_CTRL_PCKT_SIZE); ssize_t rply_size; /* reply packet size */ int pkt; int retry = 0; diff --git a/pptpgre.c b/pptpgre.c index 8402d4a..eea5c23 100644 --- a/pptpgre.c +++ b/pptpgre.c @@ -94,6 +94,7 @@ int pptp_gre_init(u_int32_t call_id_pair, int pty_fd, struct in_addr *inetaddrs) addr.sin_port = 0; if (connect(gre_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { syslog(LOG_ERR, "GRE: connect() failed: %s", strerror(errno)); + close(gre_fd); return -1; } diff --git a/pptpmanager.c b/pptpmanager.c index 7d7f0d1..53cfc5b 100644 --- a/pptpmanager.c +++ b/pptpmanager.c @@ -416,7 +416,7 @@ static int createHostSocket(int *hostSocket) #endif /* create the master socket and check it worked */ - if ((*hostSocket = vrf_socket(vrf, AF_INET, SOCK_STREAM, 0)) == 0) + if ((*hostSocket = vrf_socket(vrf, AF_INET, SOCK_STREAM, 0)) <= 0) return -1; /* set master socket to allow daemon to be restarted with connections active */ diff --git a/pqueue.c b/pqueue.c index 6059c56..c47dd81 100644 --- a/pqueue.c +++ b/pqueue.c @@ -136,6 +136,7 @@ int pqueue_add (int seq, unsigned char *packet, int packlen) { if (point->seq == seq) { // queue already contains this packet syslog(LOG_WARNING, "discarding duplicate packet %d", seq); + pqueue_del(newent); return -1; } if (point->seq > seq) { ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk