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

Proposed by Alfonso Sanchez-Beato on 2015-06-12
Status: Merged
Approved by: Manuel de la Peña on 2015-06-12
Approved revision: 99
Merged at revision: 99
Proposed branch: lp:~alfonsosanchezbeato/nuntium/fix-decode-crash
Merge into: lp:nuntium/packaging
Diff against target: 430 lines (+156/-63)
6 files modified
cmd/nuntium/mediator.go (+31/-23)
debian/changelog (+9/-0)
mms/decoder.go (+72/-37)
mms/mms.go (+1/-0)
ofono/modem.go (+19/-3)
ofono/push_decode_test.go (+24/-0)
To merge this branch: bzr merge lp:~alfonsosanchezbeato/nuntium/fix-decode-crash
Reviewer Review Type Date Requested Status
Manuel de la Peña (community) 2015-06-12 Approve on 2015-06-12
Review via email: mp+261847@code.launchpad.net

Commit message

* 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

Description of the change

* 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

To post a comment you must log in.
Manuel de la Peña (mandel) wrote :

Review was already done in github, looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/nuntium/mediator.go'
2--- cmd/nuntium/mediator.go 2014-11-06 05:20:46 +0000
3+++ cmd/nuntium/mediator.go 2015-06-12 13:14:10 +0000
4@@ -38,9 +38,7 @@
5 modem *ofono.Modem
6 telepathyService *telepathy.MMSService
7 NewMNotificationInd chan *mms.MNotificationInd
8- NewMNotifyRespInd chan *mms.MNotifyRespInd
9 NewMSendReq chan *mms.MSendReq
10- NewMNotifyRespIndFile chan string
11 NewMSendReqFile chan struct{ filePath, uuid string }
12 outMessage chan *telepathy.OutgoingMessage
13 terminate chan bool
14@@ -58,8 +56,6 @@
15 func NewMediator(modem *ofono.Modem) *Mediator {
16 mediator := &Mediator{modem: modem}
17 mediator.NewMNotificationInd = make(chan *mms.MNotificationInd)
18- mediator.NewMNotifyRespInd = make(chan *mms.MNotifyRespInd)
19- mediator.NewMNotifyRespIndFile = make(chan string)
20 mediator.NewMSendReq = make(chan *mms.MSendReq)
21 mediator.NewMSendReqFile = make(chan struct{ filePath, uuid string })
22 mediator.outMessage = make(chan *telepathy.OutgoingMessage)
23@@ -87,10 +83,6 @@
24 } else {
25 go mediator.getMRetrieveConf(mNotificationInd)
26 }
27- case mNotifyRespInd := <-mediator.NewMNotifyRespInd:
28- go mediator.handleMNotifyRespInd(mNotifyRespInd)
29- case mNotifyRespIndFilePath := <-mediator.NewMNotifyRespIndFile:
30- go mediator.sendMNotifyRespInd(mNotifyRespIndFilePath)
31 case msg := <-mediator.outMessage:
32 go mediator.handleOutgoingMessage(msg)
33 case mSendReq := <-mediator.NewMSendReq:
34@@ -126,8 +118,6 @@
35 close(mediator.NewMNotificationInd)
36 close(mediator.NewMRetrieveConf)
37 close(mediator.NewMRetrieveConfFile)
38- close(mediator.NewMNotifyRespInd)
39- close(mediator.NewMNotifyRespIndFile)
40 close(mediator.NewMSendReq)
41 close(mediator.NewMSendReqFile)
42 */
43@@ -163,12 +153,14 @@
44 defer mediator.contextLock.Unlock()
45
46 var proxy ofono.ProxyInfo
47+ var mmsContext ofono.OfonoContext
48
49 if mNotificationInd.IsLocal() {
50 log.Print("This is a local test, skipping context activation and proxy settings")
51 } else {
52+ var err error
53 preferredContext, _ := mediator.telepathyService.GetPreferredContext()
54- mmsContext, err := mediator.modem.ActivateMMSContext(preferredContext)
55+ mmsContext, err = mediator.modem.ActivateMMSContext(preferredContext)
56 if err != nil {
57 log.Print("Cannot activate ofono context: ", err)
58 return
59@@ -210,7 +202,12 @@
60 }
61
62 if !mNotificationInd.IsLocal() {
63- mediator.NewMNotifyRespInd <- mNotifyRespInd
64+ // TODO deferred case
65+ filePath := mediator.handleMNotifyRespInd(mNotifyRespInd)
66+ if filePath == "" {
67+ return
68+ }
69+ mediator.sendMNotifyRespInd(filePath, &mmsContext)
70 } else {
71 log.Print("This is a local test, skipping m-notifyresp.ind")
72 }
73@@ -246,36 +243,47 @@
74 return mRetrieveConf, nil
75 }
76
77-func (mediator *Mediator) handleMNotifyRespInd(mNotifyRespInd *mms.MNotifyRespInd) {
78+func (mediator *Mediator) handleMNotifyRespInd(mNotifyRespInd *mms.MNotifyRespInd) string {
79 f, err := storage.CreateResponseFile(mNotifyRespInd.UUID)
80 if err != nil {
81 log.Print("Unable to create m-notifyresp.ind file for ", mNotifyRespInd.UUID)
82- return
83+ return ""
84 }
85 enc := mms.NewEncoder(f)
86 if err := enc.Encode(mNotifyRespInd); err != nil {
87 log.Print("Unable to encode m-notifyresp.ind for ", mNotifyRespInd.UUID)
88 f.Close()
89- return
90+ return ""
91 }
92 filePath := f.Name()
93 if err := f.Sync(); err != nil {
94 log.Print("Error while syncing", f.Name(), ": ", err)
95- return
96+ return ""
97 }
98 if err := f.Close(); err != nil {
99 log.Print("Error while closing", f.Name(), ": ", err)
100- return
101+ return ""
102 }
103 log.Printf("Created %s to handle m-notifyresp.ind for %s", filePath, mNotifyRespInd.UUID)
104- mediator.NewMNotifyRespIndFile <- filePath
105+ return filePath
106 }
107
108-func (mediator *Mediator) sendMNotifyRespInd(mNotifyRespIndFile string) {
109- defer os.Remove(mNotifyRespIndFile)
110-
111- if _, err := mediator.uploadFile(mNotifyRespIndFile); err != nil {
112- log.Printf("Cannot upload m-notifyresp.ind encoded file %s to message center: %s", mNotifyRespIndFile, err)
113+func (mediator *Mediator) sendMNotifyRespInd(filePath string, mmsContext *ofono.OfonoContext) {
114+ defer os.Remove(filePath)
115+
116+ proxy, err := mmsContext.GetProxy()
117+ if err != nil {
118+ log.Println("Cannot retrieve MMS proxy setting", err)
119+ return
120+ }
121+ msc, err := mmsContext.GetMessageCenter()
122+ if err != nil {
123+ log.Println("Cannot retrieve MMSC setting", err)
124+ return
125+ }
126+
127+ if _, err := mms.Upload(filePath, msc, proxy.Host, int32(proxy.Port)); err != nil {
128+ log.Printf("Cannot upload m-notifyresp.ind encoded file %s to message center: %s", filePath, err)
129 }
130 }
131
132
133=== modified file 'debian/changelog'
134--- debian/changelog 2015-06-04 12:49:28 +0000
135+++ debian/changelog 2015-06-12 13:14:10 +0000
136@@ -1,3 +1,12 @@
137+nuntium (1.4+15.10.20150612-0ubuntu1) UNRELEASED; urgency=medium
138+
139+ * Fix LP: #1461107 and make PDU decoding more robust
140+ * Fix LP: #1460012 (context opened twice on rx)
141+ * Wait between retries when opening an IP context
142+ * Set ofono's Preferred property when a context is known to work
143+
144+ -- Alfonso Sanchez-Beato (email Canonical) <alfonso.sanchez-beato@canonical.com> Fri, 12 Jun 2015 14:38:53 +0200
145+
146 nuntium (1.4+15.10.20150604-0ubuntu1) wily; urgency=medium
147
148 [ Alfonso Sanchez-Beato (email Canonical) ]
149
150=== modified file 'mms/decoder.go'
151--- mms/decoder.go 2015-06-01 06:38:12 +0000
152+++ mms/decoder.go 2015-06-12 13:14:10 +0000
153@@ -23,6 +23,7 @@
154
155 import (
156 "fmt"
157+ "log"
158 "reflect"
159 )
160
161@@ -36,6 +37,24 @@
162 log string
163 }
164
165+func (dec *MMSDecoder) setPduField(pdu *reflect.Value, name string, v interface{},
166+ setter func(*reflect.Value, interface{})) {
167+
168+ if name != "" {
169+ field := pdu.FieldByName(name)
170+ if field.IsValid() {
171+ setter(&field, v)
172+ dec.log = dec.log + fmt.Sprintf("Setting %s to %s\n", name, v)
173+ } else {
174+ log.Println("Field", name, "not in decoding structure")
175+ }
176+ }
177+}
178+
179+func setterString(field *reflect.Value, v interface{}) { field.SetString(v.(string)) }
180+func setterUint64(field *reflect.Value, v interface{}) { field.SetUint(v.(uint64)) }
181+func setterSlice(field *reflect.Value, v interface{}) { field.SetBytes(v.([]byte)) }
182+
183 func (dec *MMSDecoder) ReadEncodedString(reflectedPdu *reflect.Value, hdr string) (string, error) {
184 var length uint64
185 var err error
186@@ -192,10 +211,8 @@
187 return "", fmt.Errorf("reached end of data while trying to read string: %s", dec.Data[begin:])
188 }
189 v := string(dec.Data[begin:dec.Offset])
190- if hdr != "" {
191- reflectedPdu.FieldByName(hdr).SetString(v)
192- dec.log = dec.log + fmt.Sprintf("Setting %s to %s\n", hdr, v)
193- }
194+ dec.setPduField(reflectedPdu, hdr, v, setterString)
195+
196 return v, nil
197 }
198
199@@ -208,39 +225,24 @@
200 }
201 */
202 v := dec.Data[dec.Offset] & 0x7F
203- if hdr != "" {
204- reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
205- dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
206- }
207+ dec.setPduField(reflectedPdu, hdr, uint64(v), setterUint64)
208+
209 return v, nil
210 }
211
212 func (dec *MMSDecoder) ReadByte(reflectedPdu *reflect.Value, hdr string) (byte, error) {
213 dec.Offset++
214 v := dec.Data[dec.Offset]
215- if hdr != "" {
216- reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
217- dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
218- }
219- return v, nil
220-}
221+ dec.setPduField(reflectedPdu, hdr, uint64(v), setterUint64)
222
223-func (dec *MMSDecoder) ReadBytes(reflectedPdu *reflect.Value, hdr string) ([]byte, error) {
224- dec.Offset++
225- v := []byte(dec.Data[dec.Offset:])
226- if hdr != "" {
227- reflectedPdu.FieldByName(hdr).SetBytes(v)
228- dec.log = dec.log + fmt.Sprintf("Setting %s to %#x == %d\n", hdr, v, v)
229- }
230 return v, nil
231 }
232
233 func (dec *MMSDecoder) ReadBoundedBytes(reflectedPdu *reflect.Value, hdr string, end int) ([]byte, error) {
234 v := []byte(dec.Data[dec.Offset:end])
235- if hdr != "" {
236- reflectedPdu.FieldByName(hdr).SetBytes(v)
237- }
238+ dec.setPduField(reflectedPdu, hdr, v, setterSlice)
239 dec.Offset = end - 1
240+
241 return v, nil
242 }
243
244@@ -257,10 +259,8 @@
245
246 value = value << 7
247 value |= uint64(dec.Data[dec.Offset] & 0x7F)
248- if hdr != "" {
249- reflectedPdu.FieldByName(hdr).SetUint(value)
250- dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, value)
251- }
252+ dec.setPduField(reflectedPdu, hdr, value, setterUint64)
253+
254 return value, nil
255 }
256
257@@ -276,10 +276,8 @@
258 default:
259 v, err = dec.ReadLongInteger(nil, "")
260 }
261- if hdr != "" {
262- reflectedPdu.FieldByName(hdr).SetUint(v)
263- dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, v)
264- }
265+ dec.setPduField(reflectedPdu, hdr, v, setterUint64)
266+
267 return v, err
268 }
269
270@@ -297,10 +295,8 @@
271 v |= uint64(dec.Data[dec.Offset])
272 }
273 dec.Offset--
274- if hdr != "" {
275- reflectedPdu.FieldByName(hdr).SetUint(uint64(v))
276- dec.log = dec.log + fmt.Sprintf("Setting %s to %d\n", hdr, v)
277- }
278+ dec.setPduField(reflectedPdu, hdr, v, setterUint64)
279+
280 return v, nil
281 }
282
283@@ -327,6 +323,44 @@
284 }
285 }
286
287+func (dec *MMSDecoder) skipFieldValue() error {
288+ switch {
289+ case dec.Data[dec.Offset+1] < LENGTH_QUOTE:
290+ l, err := dec.ReadByte(nil, "")
291+ if err != nil {
292+ return err
293+ }
294+ length := int(l)
295+ if dec.Offset+length >= len(dec.Data) {
296+ return fmt.Errorf("Bad field value length")
297+ }
298+ dec.Offset += length
299+ return nil
300+ case dec.Data[dec.Offset+1] == LENGTH_QUOTE:
301+ dec.Offset++
302+ // TODO These tests should be done in basic read functions
303+ if dec.Offset+1 >= len(dec.Data) {
304+ return fmt.Errorf("Bad uintvar")
305+ }
306+ l, err := dec.ReadUintVar(nil, "")
307+ if err != nil {
308+ return err
309+ }
310+ length := int(l)
311+ if dec.Offset+length >= len(dec.Data) {
312+ return fmt.Errorf("Bad field value length")
313+ }
314+ dec.Offset += length
315+ return nil
316+ case dec.Data[dec.Offset+1] <= TEXT_MAX:
317+ _, err := dec.ReadString(nil, "")
318+ return err
319+ }
320+ // case dec.Data[dec.Offset + 1] > TEXT_MAX
321+ _, err := dec.ReadShortInteger(nil, "")
322+ return err
323+}
324+
325 func (dec *MMSDecoder) Decode(pdu MMSReader) (err error) {
326 reflectedPdu := reflect.ValueOf(pdu).Elem()
327 moreHdrToRead := true
328@@ -444,7 +478,8 @@
329 case DATE:
330 _, err = dec.ReadLongInteger(&reflectedPdu, "Date")
331 default:
332- return fmt.Errorf("Unhandled byte: %#0x\tdec: %d\tdec.Offset: %d ... decoded so far: %s", param, param, dec.Offset)
333+ log.Printf("Skipping unrecognized header 0x%02x", param)
334+ err = dec.skipFieldValue()
335 }
336 if err != nil {
337 return err
338
339=== modified file 'mms/mms.go'
340--- mms/mms.go 2015-01-15 15:45:48 +0000
341+++ mms/mms.go 2015-06-12 13:14:10 +0000
342@@ -224,6 +224,7 @@
343 UUID string
344 Type, Version, Class, DeliveryReport byte
345 ReplyCharging, ReplyChargingDeadline byte
346+ Priority byte
347 ReplyChargingId string
348 TransactionId, ContentLocation string
349 From, Subject string
350
351=== modified file 'ofono/modem.go'
352--- ofono/modem.go 2015-05-20 12:32:08 +0000
353+++ ofono/modem.go 2015-06-12 13:14:10 +0000
354@@ -40,9 +40,10 @@
355 )
356
357 const (
358- ofonoAttachInProgressError = "org.ofono.AttachInProgress"
359- ofonoInProgressError = "org.ofono.InProgress"
360+ ofonoAttachInProgressError = "org.ofono.Error.AttachInProgress"
361+ ofonoInProgressError = "org.ofono.Error.InProgress"
362 ofonoNotAttachedError = "org.ofono.Error.NotAttached"
363+ ofonoFailedError = "org.ofono.Error.Failed"
364 )
365
366 type OfonoContext struct {
367@@ -269,8 +270,16 @@
368 }
369
370 func activationErrorNeedsWait(err error) bool {
371+ // ofonoFailedError might be due to network issues or to wrong APN configuration.
372+ // Retrying would not make sense for the latter, but we cannot distinguish
373+ // and any possible delay retrying might cause would happen only the first time
374+ // (provided we end up finding the right APN on the list so we save it as
375+ // preferred).
376 if dbusErr, ok := err.(*dbus.Error); ok {
377- return dbusErr.Name == ofonoInProgressError || dbusErr.Name == ofonoAttachInProgressError || dbusErr.Name == ofonoNotAttachedError
378+ return dbusErr.Name == ofonoInProgressError ||
379+ dbusErr.Name == ofonoAttachInProgressError ||
380+ dbusErr.Name == ofonoNotAttachedError ||
381+ dbusErr.Name == ofonoFailedError
382 }
383 return false
384 }
385@@ -286,6 +295,13 @@
386 time.Sleep(2 * time.Second)
387 }
388 } else {
389+ // If it works we set it as preferred in ofono, provided it is not
390+ // a combined context.
391+ // TODO get rid of nuntium's internal preferred setting
392+ if !context.isPreferred() && context.isTypeMMS() {
393+ obj.Call(CONNECTION_CONTEXT_INTERFACE, "SetProperty",
394+ "Preferred", dbus.Variant{true})
395+ }
396 return nil
397 }
398 }
399
400=== modified file 'ofono/push_decode_test.go'
401--- ofono/push_decode_test.go 2015-05-20 12:32:08 +0000
402+++ ofono/push_decode_test.go 2015-06-12 13:14:10 +0000
403@@ -199,3 +199,27 @@
404 c.Check(s.pdu.ContentType, Equals, mms.VND_WAP_MMS_MESSAGE)
405 c.Check(len(s.pdu.Data), Equals, 183)
406 }
407+
408+func (s *PushDecodeTestSuite) TestDecodePlayPoland(c *C) {
409+ inputBytes := []byte{
410+ 0x2e, 0x06, 0x22, 0x61, 0x70, 0x70, 0x6c, 0x69, 0x63, 0x61, 0x74, 0x69,
411+ 0x6f, 0x6e, 0x2f, 0x76, 0x6e, 0x64, 0x2e, 0x77, 0x61, 0x70, 0x2e, 0x6d,
412+ 0x6d, 0x73, 0x2d, 0x6d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x00, 0xaf,
413+ 0x84, 0x8c, 0x82, 0x98, 0x31, 0x34, 0x34, 0x32, 0x34, 0x30, 0x31, 0x33,
414+ 0x31, 0x38, 0x40, 0x6d, 0x6d, 0x73, 0x32, 0x00, 0x8d, 0x92, 0x89, 0x18,
415+ 0x80, 0x2b, 0x34, 0x38, 0x38, 0x38, 0x32, 0x30, 0x34, 0x30, 0x32, 0x32,
416+ 0x35, 0x2f, 0x54, 0x59, 0x50, 0x45, 0x3d, 0x50, 0x4c, 0x4d, 0x4e, 0x00,
417+ 0x8f, 0x81, 0x86, 0x80, 0x8a, 0x80, 0x8e, 0x03, 0x03, 0xad, 0x21, 0x88,
418+ 0x05, 0x81, 0x03, 0x03, 0xf4, 0x80, 0x83, 0x68, 0x74, 0x74, 0x70, 0x3a,
419+ 0x2f, 0x2f, 0x6d, 0x6d, 0x73, 0x63, 0x2e, 0x70, 0x6c, 0x61, 0x79, 0x2e,
420+ 0x70, 0x6c, 0x2f, 0x3f, 0x69, 0x64, 0x3d, 0x31, 0x34, 0x34, 0x32, 0x34,
421+ 0x30, 0x31, 0x33, 0x31, 0x38, 0x42, 0x00,
422+ }
423+ dec := NewDecoder(inputBytes)
424+ c.Assert(dec.Decode(s.pdu), IsNil)
425+
426+ c.Check(int(s.pdu.HeaderLength), Equals, 34)
427+ c.Check(int(s.pdu.ApplicationId), Equals, mms.PUSH_APPLICATION_ID)
428+ c.Check(s.pdu.ContentType, Equals, mms.VND_WAP_MMS_MESSAGE)
429+ c.Check(len(s.pdu.Data), Equals, 102)
430+}

Subscribers

People subscribed via source and target branches