Merge lp:~alfonsosanchezbeato/nuntium/fix-decode-crash into lp:nuntium/packaging

[Alfonso Sanchez-Beato]

* Fix LP: #1461107 and make PDU decoding more robust
* Fix LP: #1460012 (context opened twice on rx)
* Wait between retries when opening an IP context
* Set ofono's Preferred property when a context is known to work

* Fix LP: #1461107 and make PDU decoding more robust
* Fix LP: #1460012 (context opened twice on rx)
* Wait between retries when opening an IP context
* Set ofono's Preferred property when a context is known to work

[Manuel de la Peña (mandel)]

Review was already done in github, looks good.

review: Approve

=== modified file 'cmd/nuntium/mediator.go'
--- cmd/nuntium/mediator.go	2014-11-06 05:20:46 +0000
+++ cmd/nuntium/mediator.go	2015-06-12 13:14:10 +0000
@@ -38,9 +38,7 @@
 	modem                 *ofono.Modem
 	telepathyService      *telepathy.MMSService
 	NewMNotificationInd   chan *mms.MNotificationInd
-	NewMNotifyRespInd     chan *mms.MNotifyRespInd
 	NewMSendReq           chan *mms.MSendReq
-	NewMNotifyRespIndFile chan string
 	NewMSendReqFile       chan struct{ filePath, uuid string }
 	outMessage            chan *telepathy.OutgoingMessage
 	terminate             chan bool
@@ -58,8 +56,6 @@
 func NewMediator(modem *ofono.Modem) *Mediator {
 	mediator := &Mediator{modem: modem}
 	mediator.NewMNotificationInd = make(chan *mms.MNotificationInd)
-	mediator.NewMNotifyRespInd = make(chan *mms.MNotifyRespInd)
-	mediator.NewMNotifyRespIndFile = make(chan string)
 	mediator.NewMSendReq = make(chan *mms.MSendReq)
 	mediator.NewMSendReqFile = make(chan struct{ filePath, uuid string })
 	mediator.outMessage = make(chan *telepathy.OutgoingMessage)
@@ -87,10 +83,6 @@
 			} else {
 				go mediator.getMRetrieveConf(mNotificationInd)
 			}
-		case mNotifyRespInd := <-mediator.NewMNotifyRespInd:
-			go mediator.handleMNotifyRespInd(mNotifyRespInd)
-		case mNotifyRespIndFilePath := <-mediator.NewMNotifyRespIndFile:
-			go mediator.sendMNotifyRespInd(mNotifyRespIndFilePath)
 		case msg := <-mediator.outMessage:
 			go mediator.handleOutgoingMessage(msg)
 		case mSendReq := <-mediator.NewMSendReq:
@@ -126,8 +118,6 @@
 				close(mediator.NewMNotificationInd)
 				close(mediator.NewMRetrieveConf)
 				close(mediator.NewMRetrieveConfFile)
-				close(mediator.NewMNotifyRespInd)
-				close(mediator.NewMNotifyRespIndFile)
 				close(mediator.NewMSendReq)
 				close(mediator.NewMSendReqFile)
 			*/
@@ -163,12 +153,14 @@
 	defer mediator.contextLock.Unlock()
 
 	var proxy ofono.ProxyInfo
+	var mmsContext ofono.OfonoContext
 
 	if mNotificationInd.IsLocal() {
 		log.Print("This is a local test, skipping context activation and proxy settings")
 	} else {
+		var err error
 		preferredContext, _ := mediator.telepathyService.GetPreferredContext()
-		mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
+		mmsContext, err = mediator.modem.ActivateMMSContext(preferredContext)
 		if err != nil {
 			log.Print("Cannot activate ofono context: ", err)
 			return
@@ -210,7 +202,12 @@
 	}
 
 	if !mNotificationInd.IsLocal() {
-		mediator.NewMNotifyRespInd <- mNotifyRespInd
+		// TODO deferred case
+		filePath := mediator.handleMNotifyRespInd(mNotifyRespInd)
+		if filePath == "" {
+			return
+		}
+		mediator.sendMNotifyRespInd(filePath, &mmsContext)
 	} else {
 		log.Print("This is a local test, skipping m-notifyresp.ind")
 	}
@@ -246,36 +243,47 @@
 	return mRetrieveConf, nil
 }
 
-func (mediator *Mediator) handleMNotifyRespInd(mNotifyRespInd *mms.MNotifyRespInd) {
+func (mediator *Mediator) handleMNotifyRespInd(mNotifyRespInd *mms.MNotifyRespInd) string {
 	f, err := storage.CreateResponseFile(mNotifyRespInd.UUID)
 	if err != nil {
 		log.Print("Unable to create m-notifyresp.ind file for ", mNotifyRespInd.UUID)
-		return
+		return ""
 	}
 	enc := mms.NewEncoder(f)
 	if err := enc.Encode(mNotifyRespInd); err != nil {
 		log.Print("Unable to encode m-notifyresp.ind for ", mNotifyRespInd.UUID)
 		f.Close()
-		return
+		return ""
 	}
 	filePath := f.Name()
 	if err := f.Sync(); err != nil {
 		log.Print("Error while syncing", f.Name(), ": ", err)
-		return
+		return ""
 	}
 	if err := f.Close(); err != nil {
 		log.Print("Error while closing", f.Name(), ": ", err)
-		return
+		return ""
 	}
 	log.Printf("Created %s to handle m-notifyresp.ind for %s", filePath, mNotifyRespInd.UUID)
-	mediator.NewMNotifyRespIndFile <- filePath
+	return filePath
 }
 
-func (mediator *Mediator) sendMNotifyRespInd(mNotifyRespIndFile string) {
-	defer os.Remove(mNotifyRespIndFile)
-
-	if _, err := mediator.uploadFile(mNotifyRespIndFile); err != nil {
-		log.Printf("Cannot upload m-notifyresp.ind encoded file %s to message center: %s", mNotifyRespIndFile, err)
+func (mediator *Mediator) sendMNotifyRespInd(filePath string, mmsContext *ofono.OfonoContext) {
+	defer os.Remove(filePath)
+
+	proxy, err := mmsContext.GetProxy()
+	if err != nil {
+		log.Println("Cannot retrieve MMS proxy setting", err)
+		return
+	}
+	msc, err := mmsContext.GetMessageCenter()
+	if err != nil {
+		log.Println("Cannot retrieve MMSC setting", err)
+		return
+	}
+
+	if _, err := mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port)); err != nil {
+		log.Printf("Cannot upload m-notifyresp.ind encoded file %s to message center: %s", filePath, err)
 	}
 }
 

=== modified file 'debian/changelog'
--- debian/changelog	2015-06-04 12:49:28 +0000
+++ debian/changelog	2015-06-12 13:14:10 +0000
@@ -1,3 +1,12 @@
+nuntium (1.4+15.10.20150612-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Fix LP: #1461107 and make PDU decoding more robust
+  * Fix LP: #1460012 (context opened twice on rx)
+  * Wait between retries when opening an IP context
+  * Set ofono's Preferred property when a context is known to work
+
+ -- Alfonso Sanchez-Beato (email Canonical) <[e-mail]>  Fri, 12 Jun 2015 14:38:53 +0200
+
 nuntium (1.4+15.10.20150604-0ubuntu1) wily; urgency=medium
 
   [ Alfonso Sanchez-Beato (email Canonical) ]

=== modified file 'mms/decoder.go'
--- mms/decoder.go	2015-06-01 06:38:12 +0000
+++ mms/decoder.go	2015-06-12 13:14:10 +0000
@@ -23,6 +23,7 @@
 
 import (
 	"fmt"
+	"log"
 	"reflect"
 )
 
@@ -36,6 +37,24 @@
 	log    string
 }
 
+func (dec *MMSDecoder) setPduField(pdu *reflect.Value, name string, v interface{},
+	setter func(*reflect.Value, interface{})) {
+
+	if name != "" {
+		field := pdu.FieldByName(name)
+		if field.IsValid() {
+			setter(&field, v)
+			dec.log = dec.log + fmt.Sprintf("Setting %s to %s\n", name, v)
+		} else {
+			log.Println("Field", name, "not in decoding structure")
+		}
+	}
+}
+
+func setterString(field *reflect.Value, v interface{}) { field.SetString(v.(string)) }
+func setterUint64(field *reflect.Value, v interface{}) { field.SetUint(v.(uint64)) }
+func setterSlice(field *reflect.Value, v interface{})  { field.SetBytes(v.([]byte)) }
+
 func (dec *MMSDecoder) ReadEncodedString(reflectedPdu *reflect.Value, hdr string) (string, error) {
 	var length uint64
 	var err error
@@ -192,10 +211,8 @@
 		return "", fmt.Errorf("reached end of data while trying to read string: %s", dec.Data[begin:])
 	}
 	v := string(dec.Data[begin:dec.Offset])
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetString(v)
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %s\n", hdr, v)
-	}
+	dec.setPduField(reflectedPdu, hdr, v, setterString)
+
 	return v, nil
 }
 
@@ -208,39 +225,24 @@
 		}
 	*/
 	v := dec.Data[dec.Offset] & 0x7F
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
-	}
+	dec.setPduField(reflectedPdu, hdr, uint64(v), setterUint64)
+
 	return v, nil
 }
 
 func (dec *MMSDecoder) ReadByte(reflectedPdu *reflect.Value, hdr string) (byte, error) {
 	dec.Offset++
 	v := dec.Data[dec.Offset]
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
-	}
-	return v, nil
-}
+	dec.setPduField(reflectedPdu, hdr, uint64(v), setterUint64)
 
-func (dec *MMSDecoder) ReadBytes(reflectedPdu *reflect.Value, hdr string) ([]byte, error) {
-	dec.Offset++
-	v := []byte(dec.Data[dec.Offset:])
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetBytes(v)
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
-	}
 	return v, nil
 }
 
 func (dec *MMSDecoder) ReadBoundedBytes(reflectedPdu *reflect.Value, hdr string, end int) ([]byte, error) {
 	v := []byte(dec.Data[dec.Offset:end])
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetBytes(v)
-	}
+	dec.setPduField(reflectedPdu, hdr, v, setterSlice)
 	dec.Offset = end - 1
+
 	return v, nil
 }
 
@@ -257,10 +259,8 @@
 
 	value = value << 7
 	value |= uint64(dec.Data[dec.Offset] & 0x7F)
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetUint(value)
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, value)
-	}
+	dec.setPduField(reflectedPdu, hdr, value, setterUint64)
+
 	return value, nil
 }
 
@@ -276,10 +276,8 @@
 	default:
 		v, err = dec.ReadLongInteger(nil, "")
 	}
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetUint(v)
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, v)
-	}
+	dec.setPduField(reflectedPdu, hdr, v, setterUint64)
+
 	return v, err
 }
 
@@ -297,10 +295,8 @@
 		v |= uint64(dec.Data[dec.Offset])
 	}
 	dec.Offset--
-	if hdr != "" {
-		reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
-		dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, v)
-	}
+	dec.setPduField(reflectedPdu, hdr, v, setterUint64)
+
 	return v, nil
 }
 
@@ -327,6 +323,44 @@
 	}
 }
 
+func (dec *MMSDecoder) skipFieldValue() error {
+	switch {
+	case dec.Data[dec.Offset+1] < LENGTH_QUOTE:
+		l, err := dec.ReadByte(nil, "")
+		if err != nil {
+			return err
+		}
+		length := int(l)
+		if dec.Offset+length >= len(dec.Data) {
+			return fmt.Errorf("Bad field value length")
+		}
+		dec.Offset += length
+		return nil
+	case dec.Data[dec.Offset+1] == LENGTH_QUOTE:
+		dec.Offset++
+		// TODO These tests should be done in basic read functions
+		if dec.Offset+1 >= len(dec.Data) {
+			return fmt.Errorf("Bad uintvar")
+		}
+		l, err := dec.ReadUintVar(nil, "")
+		if err != nil {
+			return err
+		}
+		length := int(l)
+		if dec.Offset+length >= len(dec.Data) {
+			return fmt.Errorf("Bad field value length")
+		}
+		dec.Offset += length
+		return nil
+	case dec.Data[dec.Offset+1] <= TEXT_MAX:
+		_, err := dec.ReadString(nil, "")
+		return err
+	}
+	// case dec.Data[dec.Offset + 1] > TEXT_MAX
+	_, err := dec.ReadShortInteger(nil, "")
+	return err
+}
+
 func (dec *MMSDecoder) Decode(pdu MMSReader) (err error) {
 	reflectedPdu := reflect.ValueOf(pdu).Elem()
 	moreHdrToRead := true
@@ -444,7 +478,8 @@
 		case DATE:
 			_, err = dec.ReadLongInteger(&reflectedPdu, "Date")
 		default:
-			return fmt.Errorf("Unhandled byte: %#0x\tdec: %d\tdec.Offset: %d ... decoded so far: %s", param, param, dec.Offset)
+			log.Printf("Skipping unrecognized header 0x%02x", param)
+			err = dec.skipFieldValue()
 		}
 		if err != nil {
 			return err

=== modified file 'mms/mms.go'
--- mms/mms.go	2015-01-15 15:45:48 +0000
+++ mms/mms.go	2015-06-12 13:14:10 +0000
@@ -224,6 +224,7 @@
 	UUID                                 string
 	Type, Version, Class, DeliveryReport byte
 	ReplyCharging, ReplyChargingDeadline byte
+	Priority                             byte
 	ReplyChargingId                      string
 	TransactionId, ContentLocation       string
 	From, Subject                        string

=== modified file 'ofono/modem.go'
--- ofono/modem.go	2015-05-20 12:32:08 +0000
+++ ofono/modem.go	2015-06-12 13:14:10 +0000
@@ -40,9 +40,10 @@
 )
 
 const (
-	ofonoAttachInProgressError = "org.ofono.AttachInProgress"
-	ofonoInProgressError       = "org.ofono.InProgress"
+	ofonoAttachInProgressError = "org.ofono.Error.AttachInProgress"
+	ofonoInProgressError       = "org.ofono.Error.InProgress"
 	ofonoNotAttachedError      = "org.ofono.Error.NotAttached"
+	ofonoFailedError           = "org.ofono.Error.Failed"
 )
 
 type OfonoContext struct {
@@ -269,8 +270,16 @@
 }
 
 func activationErrorNeedsWait(err error) bool {
+	// ofonoFailedError might be due to network issues or to wrong APN configuration.
+	// Retrying would not make sense for the latter, but we cannot distinguish
+	// and any possible delay retrying might cause would happen only the first time
+	// (provided we end up finding the right APN on the list so we save it as
+	// preferred).
 	if dbusErr, ok := err.(*dbus.Error); ok {
-		return dbusErr.Name == ofonoInProgressError || dbusErr.Name == ofonoAttachInProgressError || dbusErr.Name == ofonoNotAttachedError
+		return dbusErr.Name == ofonoInProgressError ||
+			dbusErr.Name == ofonoAttachInProgressError ||
+			dbusErr.Name == ofonoNotAttachedError ||
+			dbusErr.Name == ofonoFailedError
 	}
 	return false
 }
@@ -286,6 +295,13 @@
 				time.Sleep(2 * time.Second)
 			}
 		} else {
+			// If it works we set it as preferred in ofono, provided it is not
+			// a combined context.
+			// TODO get rid of nuntium's internal preferred setting
+			if !context.isPreferred() && context.isTypeMMS() {
+				obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty",
+					"Preferred", dbus.Variant{true})
+			}
 			return nil
 		}
 	}

=== modified file 'ofono/push_decode_test.go'
--- ofono/push_decode_test.go	2015-05-20 12:32:08 +0000
+++ ofono/push_decode_test.go	2015-06-12 13:14:10 +0000
@@ -199,3 +199,27 @@
 	c.Check(s.pdu.ContentType, Equals, mms.VND_WAP_MMS_MESSAGE)
 	c.Check(len(s.pdu.Data), Equals, 183)
 }
+
+func (s *PushDecodeTestSuite) TestDecodePlayPoland(c *C) {
+	inputBytes := []byte{
+		0x2e, 0x06, 0x22, 0x61, 0x70, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x69,
+		0x6f, 0x6e, 0x2f, 0x76, 0x6e, 0x64, 0x2e, 0x77, 0x61, 0x70, 0x2e, 0x6d,
+		0x6d, 0x73, 0x2d, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x00, 0xaf,
+		0x84, 0x8c, 0x82, 0x98, 0x31, 0x34, 0x34, 0x32, 0x34, 0x30, 0x31, 0x33,
+		0x31, 0x38, 0x40, 0x6d, 0x6d, 0x73, 0x32, 0x00, 0x8d, 0x92, 0x89, 0x18,
+		0x80, 0x2b, 0x34, 0x38, 0x38, 0x38, 0x32, 0x30, 0x34, 0x30, 0x32, 0x32,
+		0x35, 0x2f, 0x54, 0x59, 0x50, 0x45, 0x3d, 0x50, 0x4c, 0x4d, 0x4e, 0x00,
+		0x8f, 0x81, 0x86, 0x80, 0x8a, 0x80, 0x8e, 0x03, 0x03, 0xad, 0x21, 0x88,
+		0x05, 0x81, 0x03, 0x03, 0xf4, 0x80, 0x83, 0x68, 0x74, 0x74, 0x70, 0x3a,
+		0x2f, 0x2f, 0x6d, 0x6d, 0x73, 0x63, 0x2e, 0x70, 0x6c, 0x61, 0x79, 0x2e,
+		0x70, 0x6c, 0x2f, 0x3f, 0x69, 0x64, 0x3d, 0x31, 0x34, 0x34, 0x32, 0x34,
+		0x30, 0x31, 0x33, 0x31, 0x38, 0x42, 0x00,
+	}
+	dec := NewDecoder(inputBytes)
+	c.Assert(dec.Decode(s.pdu), IsNil)
+
+	c.Check(int(s.pdu.HeaderLength), Equals, 34)
+	c.Check(int(s.pdu.ApplicationId), Equals, mms.PUSH_APPLICATION_ID)
+	c.Check(s.pdu.ContentType, Equals, mms.VND_WAP_MMS_MESSAGE)
+	c.Check(len(s.pdu.Data), Equals, 102)
+}