- Add some fixes from CVS:

- Fix waitpid usage
- Move free of connection struct out of main loop
- Avoid using connection struct after it is freed
This commit is contained in:
Paul Howarth 2010-06-16 15:03:25 +00:00
parent 5bafe2515b
commit 96fd87783e
4 changed files with 209 additions and 2 deletions

View File

@ -0,0 +1,90 @@
Tue Jun 15 15:00:40 2010 James Cameron <quozl@laptop.org>
* pptp_ctrl.c (pptp_conn_is_dead): immediately destroying the
connection and freeing the structure has led to segmentation
faults on more recent heap implementations, since we use the
structure after it has been freed.
Defer the free of the structure until after all uses of it have
ceased, by adding a connection state for dead and terminating the
main loop once it is detected.
--- pptp_callmgr.c 2008-05-14 07:33:55.000000000 +0100
+++ pptp_callmgr.c 2010-06-15 14:32:00.478100392 +0100
@@ -167,6 +170,7 @@
do {
int rc;
fd_set read_set = call_set, write_set;
+ if (pptp_conn_is_dead(conn)) break;
FD_ZERO (&write_set);
if (pptp_conn_established(conn)) {
FD_SET (unix_sock, &read_set);
@@ -294,6 +298,7 @@
}
/* with extreme prejudice */
pptp_conn_destroy(conn);
+ pptp_conn_free(conn);
vector_destroy(call_list);
}
cleanup:
--- pptp_ctrl.c 2008-05-14 07:33:55.000000000 +0100
+++ pptp_ctrl.c 2010-06-15 14:32:00.480100647 +0100
@@ -58,8 +62,11 @@
struct PPTP_CONN {
int inet_sock;
/* Connection States */
- enum {
- CONN_IDLE, CONN_WAIT_CTL_REPLY, CONN_WAIT_STOP_REPLY, CONN_ESTABLISHED
+ enum {
+ CONN_IDLE,
+ CONN_WAIT_CTL_REPLY, CONN_WAIT_STOP_REPLY,
+ CONN_ESTABLISHED,
+ CONN_DEAD
} conn_state; /* on startup: CONN_IDLE */
/* Keep-alive states */
enum {
@@ -448,6 +457,16 @@
close(conn->inet_sock);
/* deallocate */
vector_destroy(conn->call);
+ conn->conn_state = CONN_DEAD;
+}
+
+int pptp_conn_is_dead(PPTP_CONN * conn)
+{
+ return conn->conn_state == CONN_DEAD;
+}
+
+void pptp_conn_free(PPTP_CONN * conn)
+{
free(conn);
}
@@ -1038,11 +1059,13 @@
int i;
/* "Keep Alives and Timers, 1": check connection state */
if (global.conn->conn_state != CONN_ESTABLISHED) {
- if (global.conn->conn_state == CONN_WAIT_STOP_REPLY)
+ if (global.conn->conn_state == CONN_WAIT_STOP_REPLY) {
/* hard close. */
pptp_conn_destroy(global.conn);
- else /* soft close */
- pptp_conn_close(global.conn, PPTP_STOP_NONE);
+ return;
+ }
+ /* soft close */
+ pptp_conn_close(global.conn, PPTP_STOP_NONE);
}
/* "Keep Alives and Timers, 2": check echo status */
if (global.conn->ka_state == KA_OUTSTANDING) {
--- pptp_ctrl.h 2008-05-14 07:33:55.000000000 +0100
+++ pptp_ctrl.h 2010-06-15 14:32:00.864975405 +0100
@@ -33,6 +33,8 @@
void pptp_call_close(PPTP_CONN * conn, PPTP_CALL * call);
/* hard close. */
void pptp_call_destroy(PPTP_CONN *conn, PPTP_CALL *call);
+int pptp_conn_is_dead(PPTP_CONN * conn);
+void pptp_conn_free(PPTP_CONN * conn);
/* soft close. Will callback on completion. */
void pptp_conn_close(PPTP_CONN * conn, u_int8_t close_reason);
/* hard close */

View File

@ -0,0 +1,83 @@
Fri Jun 4 10:54:04 2010 Jan Just Keijser <jan.just.keijser@gmail.com>
* pptp_ctrl.c: check for failure return by pptp_send_ctrl_packet
and avoid using freed struct conn.
--- pptp_ctrl.c 2010-06-15 15:05:46.743913798 +0100
+++ pptp_ctrl.c 2010-06-15 14:32:00.480100647 +0100
@@ -396,9 +400,10 @@
/* don't check state against WAIT_DISCONNECT... allow multiple disconnect
* requests to be made.
*/
- pptp_send_ctrl_packet(conn, &rqst, sizeof(rqst));
- pptp_reset_timer();
- call->state.pns = PNS_WAIT_DISCONNECT;
+ if (pptp_send_ctrl_packet(conn, &rqst, sizeof(rqst))) {
+ pptp_reset_timer();
+ call->state.pns = PNS_WAIT_DISCONNECT;
+ }
/* call structure will be freed when we have confirmation of disconnect. */
}
@@ -431,9 +436,10 @@
pptp_call_close(conn, vector_get_Nth(conn->call, i));
/* now close connection */
log("Closing PPTP connection");
- pptp_send_ctrl_packet(conn, &rqst, sizeof(rqst));
- pptp_reset_timer(); /* wait 60 seconds for reply */
- conn->conn_state = CONN_WAIT_STOP_REPLY;
+ if (pptp_send_ctrl_packet(conn, &rqst, sizeof(rqst))) {
+ pptp_reset_timer(); /* wait 60 seconds for reply */
+ conn->conn_state = CONN_WAIT_STOP_REPLY;
+ }
return;
}
@@ -733,8 +739,8 @@
reply.version = packet->version;
/* protocol version not supported */
reply.result_code = hton8(5);
- pptp_send_ctrl_packet(conn, &reply, sizeof(reply));
- pptp_reset_timer(); /* give sender a chance for a retry */
+ if (pptp_send_ctrl_packet(conn, &reply, sizeof(reply)))
+ pptp_reset_timer(); /* give sender a chance for a retry */
} else { /* same or greater version */
if (pptp_send_ctrl_packet(conn, &reply, sizeof(reply))) {
conn->conn_state = CONN_ESTABLISHED;
@@ -841,8 +847,8 @@
hton8(1), hton8(PPTP_GENERAL_ERROR_NONE), 0
};
logecho( PPTP_ECHO_RQST);
- pptp_send_ctrl_packet(conn, &reply, sizeof(reply));
- pptp_reset_timer();
+ if (pptp_send_ctrl_packet(conn, &reply, sizeof(reply)))
+ pptp_reset_timer();
break;
}
/* ----------- OUTGOING CALL MESSAGES ------------ */
@@ -928,9 +935,10 @@
vector_search(conn->call, ntoh16(packet->call_id), &call);
if (call->callback != NULL)
call->callback(conn, call, CALL_CLOSE_RQST);
- pptp_send_ctrl_packet(conn, &reply, sizeof(reply));
- pptp_call_destroy(conn, call);
- log("Call closed (RQST) (call id %d)", (int) call->call_id);
+ if (pptp_send_ctrl_packet(conn, &reply, sizeof(reply))) {
+ pptp_call_destroy(conn, call);
+ log("Call closed (RQST) (call id %d)", (int) call->call_id);
+ }
}
break;
}
@@ -1067,8 +1075,9 @@
} else { /* ka_state == NONE */ /* send keep-alive */
struct pptp_echo_rqst rqst = {
PPTP_HEADER_CTRL(PPTP_ECHO_RQST), hton32(global.conn->ka_id) };
- pptp_send_ctrl_packet(global.conn, &rqst, sizeof(rqst));
- global.conn->ka_state = KA_OUTSTANDING;
+ if (pptp_send_ctrl_packet(global.conn, &rqst, sizeof(rqst))) {
+ global.conn->ka_state = KA_OUTSTANDING;
+ }
}
/* check incoming/outgoing call states for !IDLE && !ESTABLISHED */
for (i = 0; i < vector_size(global.conn->call); i++) {

16
pptp-1.7.2-waitpid.patch Normal file
View File

@ -0,0 +1,16 @@
Tue Jun 15 15:02:28 2010 James Cameron <quozl@us.netrek.org>
* pptp.c (open_callmgr): fix usage of status returned by waitpid;
it must be wrapped by WEXITSTATUS to shift bits as required.
--- pptp.c 2010-06-15 14:35:20.265852021 +0100
+++ pptp.c 2010-06-15 14:32:00.478100392 +0100
@@ -475,7 +475,7 @@
}
default: /* parent */
waitpid(pid, &status, 0);
- if (status!= 0)
+ if (WEXITSTATUS(status) != 0)
fatal("Call manager exited with error %d", status);
break;
}

View File

@ -1,6 +1,6 @@
Name: pptp
Version: 1.7.2
Release: 8%{?dist}
Release: 9%{?dist}
Summary: Point-to-Point Tunneling Protocol (PPTP) Client
Group: Applications/Internet
License: GPLv2+
@ -12,6 +12,9 @@ Patch2: pptp-1.7.2-pptpsetup.patch
Patch3: pptp-1.7.2-makedeps.patch
Patch4: pptp-1.7.2-pptpsetup-encrypt.patch
Patch5: pptp-1.7.2-pptpsetup-mppe.patch
Patch6: pptp-1.7.2-waitpid.patch
Patch7: pptp-1.7.2-conn-free.patch
Patch8: pptp-1.7.2-conn-free2.patch
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Requires: ppp >= 2.4.2, /sbin/ip
@ -52,6 +55,15 @@ tunnels.
# Fedora releases and EL >= 5 include MPPE support out of the box (#502967)
%patch5 -p1 -b .mppe
# Fix waitpid usage (upstream patch)
%patch6 -p0 -b .waitpid
# Move free of connection struct out of main loop (upstream patch)
%patch7 -p0 -b .conn-free
# Avoid using connection struct after it is freed (upstream patch)
%patch8 -p0 -b .conn-free2
# Pacify rpmlint
%{__perl} -pi -e 's/install -o root -m 555 pptp/install -m 755 pptp/;' Makefile
@ -73,7 +85,7 @@ tunnels.
%{_sbindir}/pptp
%{_mandir}/man8/pptp.8*
%dir %attr(750,root,root) %{_localstatedir}/run/pptp/
# /etc/ppp is hardcoded instead of using %{_sysconfdir}/ppp because the
# /etc/ppp is hardcoded instead of using %%{_sysconfdir}/ppp because the
# Fedora ppp package hardcodes the directory name
%config(noreplace) /etc/ppp/options.pptp
@ -83,6 +95,12 @@ tunnels.
%{_mandir}/man8/pptpsetup.8*
%changelog
* Wed Jun 16 2010 Paul Howarth <paul@city-fan.org> 1.7.2-9
- Add some fixes from CVS:
- Fix waitpid usage
- Move free of connection struct out of main loop
- Avoid using connection struct after it is freed
* Thu Sep 24 2009 Paul Howarth <paul@city-fan.org> 1.7.2-8
- Split pptpsetup into subpackage to avoid perl dependency (#524972)