Merge pull request #223 from rod-hynes/master

Recover from panics

psiphon/common/throttled.go

@@ -57,11 +57,9 @@ type RateLimits struct {
 // The underlying rate limiter uses the token bucket algorithm to
 // calculate delay times for read and write operations.
 type ThrottledConn struct {
-	// https://golang.org/src/sync/atomic/doc.go#L50
-	// On both ARM and x86-32, it is the caller's responsibility to arrange for 64-bit
-	// alignment of 64-bit words accessed atomically. The first word in a global
-	// variable or in an allocated struct or slice can be relied upon to be
-	// 64-bit aligned.
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	unlimitedReadBytes  int64
 	unlimitedWriteBytes int64
 	limitingReads       int32

psiphon/server/api.go

@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"net"
 	"regexp"
+	"runtime/debug"
 	"strconv"
 	"strings"
 	"unicode"
@@ -71,6 +72,29 @@ func sshAPIRequestHandler(
 			fmt.Errorf("invalid payload for request name: %s: %s", name, err))
 	}
 
+	return dispatchAPIRequestHandler(support, geoIPData, name, params)
+}
+
+// dispatchAPIRequestHandler is the common dispatch point for both
+// web and SSH API requests.
+func dispatchAPIRequestHandler(
+	support *SupportServices,
+	geoIPData GeoIPData,
+	name string,
+	params requestJSONObject) (response []byte, reterr error) {
+
+	// Recover from and log any unexpected panics caused by user input
+	// handling bugs. User inputs should be properly validated; this
+	// mechanism is only a last resort to prevent the process from
+	// terminating in the case of a bug.
+	defer func() {
+		if e := recover(); e != nil {
+			reterr = common.ContextError(
+				fmt.Errorf(
+					"request handler panic: %s: %s", e, debug.Stack()))
+		}
+	}()
+
 	switch name {
 	case common.PSIPHON_API_HANDSHAKE_REQUEST_NAME:
 		return handshakeAPIRequestHandler(support, geoIPData, params)

psiphon/server/dns.go

@@ -41,6 +41,9 @@ const (
 // "/etc/resolv.conf" on platforms where it is available; and
 // otherwise using a default value.
 type DNSResolver struct {
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	lastReloadTime int64
 	common.ReloadableFile
 	isReloading int32

psiphon/server/meek.go

@@ -443,6 +443,9 @@ func (server *MeekServer) terminateConnection(
 }
 
 type meekSession struct {
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	lastActivity        int64
 	clientConn          *meekConn
 	meekProtocolVersion int

psiphon/server/net.go

@@ -160,6 +160,9 @@ func (entry *LRUConnsEntry) Touch() {
 // either a successful read or write.
 //
 type ActivityMonitoredConn struct {
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	startTime            int64
 	lastReadActivityTime int64
 	net.Conn

psiphon/server/safetyNet.go

@@ -104,7 +104,7 @@ type jwtBody struct {
 	// exist and the default values assigned when
 	// unmarshalling into the corresponding non-pointer
 	// struct would cause non-existing fields to be logged
-	// in a manner that would make it impossible to distinguise
+	// in a manner that would make it impossible to distinguish
 	// between a non-existing field and field of default value
 	BasicIntegrity             *bool     `json:"basicIntegrity"`
 	CtsProfileMatch            *bool     `json:"ctsProfileMatch"`

psiphon/server/server_test.go

@@ -113,10 +113,10 @@ func sendNotificationReceived(c chan<- struct{}) {
 	}
 }
 
-func waitOnNotification(c <-chan struct{}, t *testing.T, timeout <-chan time.Time, timeoutMessage string) {
+func waitOnNotification(t *testing.T, c, timeoutSignal <-chan struct{}, timeoutMessage string) {
 	select {
 	case <-c:
-	case <-timeout:
+	case <-timeoutSignal:
 		t.Fatalf(timeoutMessage)
 	}
 }
@@ -265,7 +265,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 	psiphon.SetNoticeOutput(psiphon.NewNoticeReceiver(
 		func(notice []byte) {
 
-			fmt.Printf("%s\n", string(notice))
+			//fmt.Printf("%s\n", string(notice))
 
 			noticeType, payload, err := psiphon.GetNotice(notice)
 			if err != nil {
@@ -324,12 +324,17 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 	// Test: tunnels must be established, and correct homepage
 	// must be received, within 30 seconds
 
-	establishedTimeout := time.NewTimer(30 * time.Second)
+	timeoutSignal := make(chan struct{})
+	go func() {
+		timer := time.NewTimer(30 * time.Second)
+		<-timer.C
+		close(timeoutSignal)
+	}()
 
-	waitOnNotification(tunnelsEstablished, t, establishedTimeout.C, "tunnel establish timeout exceeded")
-	waitOnNotification(homepageReceived, t, establishedTimeout.C, "homepage received timeout exceeded")
-	waitOnNotification(verificationRequired, t, establishedTimeout.C, "verification required timeout exceeded")
-	waitOnNotification(verificationCompleted, t, establishedTimeout.C, "verification completed timeout exceeded")
+	waitOnNotification(t, tunnelsEstablished, timeoutSignal, "tunnel establish timeout exceeded")
+	waitOnNotification(t, homepageReceived, timeoutSignal, "homepage received timeout exceeded")
+	waitOnNotification(t, verificationRequired, timeoutSignal, "verification required timeout exceeded")
+	waitOnNotification(t, verificationCompleted, timeoutSignal, "verification completed timeout exceeded")
 
 	// Test: tunneled web site fetch
 

psiphon/server/tunnelServer.go

@@ -551,6 +551,9 @@ type sshClient struct {
 }
 
 type trafficState struct {
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	bytesUp                        int64
 	bytesDown                      int64
 	concurrentPortForwardCount     int64

psiphon/server/udp.go

@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"runtime/debug"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -100,6 +101,17 @@ func (mux *udpPortForwardMultiplexer) run() {
 	// When the client disconnects or the server shuts down, the channel will close and
 	// readUdpgwMessage will exit with EOF.
 
+	// Recover from and log any unexpected panics caused by udpgw input handling bugs.
+	// Note: this covers the run() goroutine only and not relayDownstream() goroutines.
+	defer func() {
+		if e := recover(); e != nil {
+			err := common.ContextError(
+				fmt.Errorf(
+					"udpPortForwardMultiplexer panic: %s: %s", e, debug.Stack()))
+			log.WithContextFields(LogFields{"error": err}).Warning("run failed")
+		}
+	}()
+
 	buffer := make([]byte, udpgwProtocolMaxMessageSize)
 	for {
 		// Note: message.packet points to the reusable memory in "buffer".
@@ -268,6 +280,9 @@ func (mux *udpPortForwardMultiplexer) removePortForward(connID uint16) {
 }
 
 type udpPortForward struct {
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	bytesUp      int64
 	bytesDown    int64
 	connID       uint16
@@ -302,7 +317,7 @@ func (portForward *udpPortForward) relayDownstream() {
 		if err != nil {
 			if err != io.EOF {
 				// Debug since errors such as "use of closed network connection" occur during normal operation
-				log.WithContextFields(LogFields{"error": err}).Warning("downstream UDP relay failed")
+				log.WithContextFields(LogFields{"error": err}).Debug("downstream UDP relay failed")
 			}
 			break
 		}

psiphon/server/webServer.go

@@ -225,8 +225,11 @@ func (webServer *webServer) handshakeHandler(w http.ResponseWriter, r *http.Requ
 
 	var responsePayload []byte
 	if err == nil {
-		responsePayload, err = handshakeAPIRequestHandler(
-			webServer.support, webServer.lookupGeoIPData(params), params)
+		responsePayload, err = dispatchAPIRequestHandler(
+			webServer.support,
+			webServer.lookupGeoIPData(params),
+			common.PSIPHON_API_HANDSHAKE_REQUEST_NAME,
+			params)
 	}
 
 	if err != nil {
@@ -251,8 +254,11 @@ func (webServer *webServer) connectedHandler(w http.ResponseWriter, r *http.Requ
 
 	var responsePayload []byte
 	if err == nil {
-		responsePayload, err = connectedAPIRequestHandler(
-			webServer.support, webServer.lookupGeoIPData(params), params)
+		responsePayload, err = dispatchAPIRequestHandler(
+			webServer.support,
+			webServer.lookupGeoIPData(params),
+			common.PSIPHON_API_CONNECTED_REQUEST_NAME,
+			params)
 	}
 
 	if err != nil {
@@ -270,8 +276,11 @@ func (webServer *webServer) statusHandler(w http.ResponseWriter, r *http.Request
 	params, err := convertHTTPRequestToAPIRequest(w, r, "statusData")
 
 	if err == nil {
-		_, err = statusAPIRequestHandler(
-			webServer.support, webServer.lookupGeoIPData(params), params)
+		_, err = dispatchAPIRequestHandler(
+			webServer.support,
+			webServer.lookupGeoIPData(params),
+			common.PSIPHON_API_STATUS_REQUEST_NAME,
+			params)
 	}
 
 	if err != nil {
@@ -289,8 +298,11 @@ func (webServer *webServer) clientVerificationHandler(w http.ResponseWriter, r *
 
 	var responsePayload []byte
 	if err == nil {
-		responsePayload, err = clientVerificationAPIRequestHandler(
-			webServer.support, webServer.lookupGeoIPData(params), params)
+		responsePayload, err = dispatchAPIRequestHandler(
+			webServer.support,
+			webServer.lookupGeoIPData(params),
+			common.PSIPHON_API_CLIENT_VERIFICATION_REQUEST_NAME,
+			params)
 	}
 
 	if err != nil {

psiphon/serverApi.go

@@ -44,8 +44,11 @@ import (
 // offer the Psiphon API through a web service; newer servers offer the Psiphon
 // API through SSH requests made directly through the tunnel's SSH client.
 type ServerContext struct {
-	sessionId                string
+	// Note: 64-bit ints used with atomic operations are at placed
+	// at the start of struct to ensure 64-bit alignment.
+	// (https://golang.org/pkg/sync/atomic/#pkg-note-BUG)
 	tunnelNumber             int64
+	sessionId                string
 	tunnel                   *Tunnel
 	psiphonHttpsClient       *http.Client
 	statsRegexps             *transferstats.Regexps