From a98f5ca38ee39e35962c55b500d3a02c89182997 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 7 May 2014 17:44:46 -0500 Subject: [PATCH] Cleanup a few nitpicks with recent alert additions. - Group the new read/writeVarBytes functions together to be consistent with the existing code - Modify the comments on the new read/writeVarBytes to be a little more descriptive and consistent with existing code - Use "test payload" for field name in the tests for the read/writeVarBytes functions which is more accurate - Remove reserved param from NewAlert since there is no point is having the caller deal with a reserved param - Various comment tweaks for clarity and consistency - Use camel case for fuction params for consistency - Move the NewAlert and NewAlertFromPayload functions after the receiver definitions for code layout consistency Closes #11. --- common.go | 91 +++++++++++++++++++++------------------- common_test.go | 12 +++--- msgalert.go | 105 ++++++++++++++++++++++++----------------------- msgalert_test.go | 4 +- 4 files changed, 111 insertions(+), 101 deletions(-) diff --git a/common.go b/common.go index eb337dc9..b84afa32 100644 --- a/common.go +++ b/common.go @@ -281,32 +281,6 @@ func writeElements(w io.Writer, elements ...interface{}) error { return nil } -// readVarBytes reads a variable length byte array -func readVarBytes(r io.Reader, pver uint32, maxAllowed uint32, - fieldName string) ([]byte, error) { - - count, err := readVarInt(r, pver) - if err != nil { - return nil, err - } - - // Prevent byte array larger than the max message size. It would - // be possible to cause memory exhaustion and panics without a sane - // upper bound on this count. - if count > uint64(maxAllowed) { - str := fmt.Sprintf("%s is larger than the max allowed size "+ - "[count %d, max %d]", fieldName, count, maxAllowed) - return nil, messageError("readVarBytes", str) - } - - b := make([]byte, count) - _, err = io.ReadFull(r, b) - if err != nil { - return nil, err - } - return b, nil -} - // readVarInt reads a variable length integer from r and returns it as a uint64. func readVarInt(r io.Reader, pver uint32) (uint64, error) { var b [8]byte @@ -346,21 +320,6 @@ func readVarInt(r io.Reader, pver uint32) (uint64, error) { return rv, nil } -// writeVarBytes writes a variable length byte array -func writeVarBytes(w io.Writer, pver uint32, bytes []byte) error { - slen := uint64(len(bytes)) - err := writeVarInt(w, pver, slen) - if err != nil { - return err - } - - _, err = w.Write(bytes) - if err != nil { - return err - } - return nil -} - // writeVarInt serializes val to w using a variable number of bytes depending // on its value. func writeVarInt(w io.Writer, pver uint32, val uint64) error { @@ -420,7 +379,7 @@ func VarIntSerializeSize(val uint64) int { // string, and the bytes that represent the string itself. An error is returned // if the length is greater than the maximum block payload size, since it would // not be possible to put a varString of that size into a block anyways and it -// also helps protect against memory exhuastion attacks and forced panics +// also helps protect against memory exhaustion attacks and forced panics // through malformed messages. func readVarString(r io.Reader, pver uint32) (string, error) { count, err := readVarInt(r, pver) @@ -459,6 +418,54 @@ func writeVarString(w io.Writer, pver uint32, str string) error { return nil } +// readVarBytes reads a variable length byte array. A byte array is encoded +// as a varInt containing the length of the array followed by the bytes +// themselves. An error is returned if the length is greater than the +// passed maxAllowed parameter which helps protect against memory exhuastion +// attacks and forced panics thorugh malformed messages. The fieldName +// parameter is only used for the error message so it provides more context in +// the error. +func readVarBytes(r io.Reader, pver uint32, maxAllowed uint32, + fieldName string) ([]byte, error) { + + count, err := readVarInt(r, pver) + if err != nil { + return nil, err + } + + // Prevent byte array larger than the max message size. It would + // be possible to cause memory exhaustion and panics without a sane + // upper bound on this count. + if count > uint64(maxAllowed) { + str := fmt.Sprintf("%s is larger than the max allowed size "+ + "[count %d, max %d]", fieldName, count, maxAllowed) + return nil, messageError("readVarBytes", str) + } + + b := make([]byte, count) + _, err = io.ReadFull(r, b) + if err != nil { + return nil, err + } + return b, nil +} + +// writeVarInt serializes a variable length byte array to w as a varInt +// containing the number of bytes, followed by the bytes themselves. +func writeVarBytes(w io.Writer, pver uint32, bytes []byte) error { + slen := uint64(len(bytes)) + err := writeVarInt(w, pver, slen) + if err != nil { + return err + } + + _, err = w.Write(bytes) + if err != nil { + return err + } + return nil +} + // randomUint64 returns a cryptographically random uint64 value. This // unexported version takes a reader primarily to ensure the error paths // can be properly tested by passing a fake reader in the tests. diff --git a/common_test.go b/common_test.go index 3a534d47..b9418974 100644 --- a/common_test.go +++ b/common_test.go @@ -539,8 +539,8 @@ func TestVarBytesWire(t *testing.T) { // Decode from wire format. rbuf := bytes.NewBuffer(test.buf) - val, err := btcwire.TstReadVarBytes(rbuf, test.pver, btcwire.MaxMessagePayload, - "alert serialized payload") + val, err := btcwire.TstReadVarBytes(rbuf, test.pver, + btcwire.MaxMessagePayload, "test payload") if err != nil { t.Errorf("readVarBytes #%d error %v", i, err) continue @@ -591,8 +591,8 @@ func TestVarBytesWireErrors(t *testing.T) { // Decode from wire format. r := newFixedReader(test.max, test.buf) - _, err = btcwire.TstReadVarBytes(r, test.pver, btcwire.MaxMessagePayload, - "alert serialized payload") + _, err = btcwire.TstReadVarBytes(r, test.pver, + btcwire.MaxMessagePayload, "test payload") if err != test.readErr { t.Errorf("readVarBytes #%d wrong error got: %v, want: %v", i, err, test.readErr) @@ -623,8 +623,8 @@ func TestVarBytesOverflowErrors(t *testing.T) { for i, test := range tests { // Decode from wire format. rbuf := bytes.NewBuffer(test.buf) - _, err := btcwire.TstReadVarBytes(rbuf, test.pver, btcwire.MaxMessagePayload, - "alert serialized payload") + _, err := btcwire.TstReadVarBytes(rbuf, test.pver, + btcwire.MaxMessagePayload, "test payload") if reflect.TypeOf(err) != reflect.TypeOf(test.err) { t.Errorf("readVarBytes #%d wrong error got: %v, "+ "want: %v", i, err, reflect.TypeOf(test.err)) diff --git a/msgalert.go b/msgalert.go index 2bd45dfd..d1026c2a 100644 --- a/msgalert.go +++ b/msgalert.go @@ -58,46 +58,49 @@ import ( // | Total (Fixed) | 45 | // ----------------------------------------------- // -// note: +// NOTE: // * string is a VarString i.e VarInt length followed by the string itself // * set is a VarInt followed by as many number of strings // * set is a VarInt followed by as many number of ints // * fixedAlertSize = 40 + 5*min(VarInt) = 40 + 5*1 = 45 - +// // Now we can define bounds on Alert size, SetCancel and SetSubVer // Fixed size of the alert payload const fixedAlertSize = 45 -// Max size of the ECDSA signature -// note: since this size is fixed and < 255, size of VarInt -// required = 1 (fits in uint8) +// maxSignatureSize is the max size of an ECDSA signature. +// NOTE: Since this size is fixed and < 255, the size of VarInt required = 1. const maxSignatureSize = 72 -// Maximum size of the alert +// maxAlertSize is the maximum size an alert. +// // MessagePayload = VarInt(Alert) + Alert + VarInt(Signature) + Signature // maxMessagePayload = maxAlertSize + max(VarInt) + maxSignatureSize + 1 const maxAlertSize = maxMessagePayload - maxSignatureSize - MaxVarIntPayload - 1 -// Maximum number of Cancel IDs from SetCancel to read +// maxCountSetCancel is the maximum number of cancel IDs that could possibly +// fit into a maximum size alert. +// // maxAlertSize = fixedAlertSize + max(SetCancel) + max(SetSubVer) + 3*(string) -// for caculating maximum number of Cancel IDs, set all other variable sizes to 0 +// for caculating maximum number of cancel IDs, set all other var sizes to 0 // maxAlertSize = fixedAlertSize + (MaxVarIntPayload-1) + x*sizeOf(int32) // x = (maxAlertSize - fixedAlertSize - MaxVarIntPayload + 1) / 4 const maxCountSetCancel = (maxAlertSize - fixedAlertSize - MaxVarIntPayload + 1) / 4 -// Maximum number of subversions from SetSubVer to read +// maxCountSetSubVer is the maximum number of subversions that could possibly +// fit into a maximum size alert. +// // maxAlertSize = fixedAlertSize + max(SetCancel) + max(SetSubVer) + 3*(string) -// for caculating maximum number of subversions, set all other variable sizes to 0 +// for caculating maximum number of subversions, set all other var sizes to 0 // maxAlertSize = fixedAlertSize + (MaxVarIntPayload-1) + x*sizeOf(string) // x = (maxAlertSize - fixedAlertSize - MaxVarIntPayload + 1) / sizeOf(string) // subversion would typically be something like "/Satoshi:0.7.2/" (15 bytes) // so assuming < 255 bytes, sizeOf(string) = sizeOf(uint8) + 255 = 256 const maxCountSetSubVer = (maxAlertSize - fixedAlertSize - MaxVarIntPayload + 1) / 256 -// Alert contains the data deserialized from the MsgAlert payload +// Alert contains the data deserialized from the MsgAlert payload. type Alert struct { - // Alert format version Version int32 @@ -144,41 +147,7 @@ type Alert struct { Reserved string } -// NewAlert returns an new Alert with values provided -func NewAlert(version int32, relayuntil int64, expiration int64, - id int32, cancel int32, setcancel []int32, minver int32, - maxver int32, setsubver []string, priority int32, comment string, - statusbar string, reserved string) *Alert { - return &Alert{ - Version: version, - RelayUntil: relayuntil, - Expiration: expiration, - ID: id, - Cancel: cancel, - SetCancel: setcancel, - MinVer: minver, - MaxVer: maxver, - SetSubVer: setsubver, - Priority: priority, - Comment: comment, - StatusBar: statusbar, - Reserved: reserved, - } -} - -// NewAlertFromPayload returns an Alert with values deserialized -// from the serializedpayload -func NewAlertFromPayload(serializedpayload []byte, pver uint32) (*Alert, error) { - var alert Alert - r := bytes.NewReader(serializedpayload) - err := alert.Deserialize(r, pver) - if err != nil { - return nil, err - } - return &alert, nil -} - -// Serialize writes a serialized byte array of the Alert +// Serialize encodes the alert to w using the alert protocol encoding format. func (alert *Alert) Serialize(w io.Writer, pver uint32) error { err := writeElements(w, &alert.Version, &alert.RelayUntil, &alert.Expiration, &alert.ID, &alert.Cancel) @@ -244,8 +213,8 @@ func (alert *Alert) Serialize(w io.Writer, pver uint32) error { return nil } -// Deserialize reads a byte array, deserializes -// it and updates the Alert +// Deserialize decodes from r into the receiver using the alert protocol +// encoding format. func (alert *Alert) Deserialize(r io.Reader, pver uint32) error { err := readElements(r, &alert.Version, &alert.RelayUntil, &alert.Expiration, &alert.ID, &alert.Cancel) @@ -316,6 +285,40 @@ func (alert *Alert) Deserialize(r io.Reader, pver uint32) error { return nil } +// NewAlert returns an new Alert with values provided. +func NewAlert(version int32, relayUntil int64, expiration int64, + id int32, cancel int32, setCancel []int32, minVer int32, + maxVer int32, setSubVer []string, priority int32, comment string, + statusBar string) *Alert { + return &Alert{ + Version: version, + RelayUntil: relayUntil, + Expiration: expiration, + ID: id, + Cancel: cancel, + SetCancel: setCancel, + MinVer: minVer, + MaxVer: maxVer, + SetSubVer: setSubVer, + Priority: priority, + Comment: comment, + StatusBar: statusBar, + Reserved: "", + } +} + +// NewAlertFromPayload returns an Alert with values deserialized from the +// serialized payload. +func NewAlertFromPayload(serializedPayload []byte, pver uint32) (*Alert, error) { + var alert Alert + r := bytes.NewReader(serializedPayload) + err := alert.Deserialize(r, pver) + if err != nil { + return nil, err + } + return &alert, nil +} + // MsgAlert implements the Message interface and defines a bitcoin alert // message. // @@ -410,9 +413,9 @@ func (msg *MsgAlert) MaxPayloadLength(pver uint32) uint32 { // NewMsgAlert returns a new bitcoin alert message that conforms to the Message // interface. See MsgAlert for details. -func NewMsgAlert(serializedpayload []byte, signature []byte) *MsgAlert { +func NewMsgAlert(serializedPayload []byte, signature []byte) *MsgAlert { return &MsgAlert{ - SerializedPayload: serializedpayload, + SerializedPayload: serializedPayload, Signature: signature, Payload: nil, } diff --git a/msgalert_test.go b/msgalert_test.go index 1ae012c9..20f8287e 100644 --- a/msgalert_test.go +++ b/msgalert_test.go @@ -283,7 +283,7 @@ func TestAlert(t *testing.T) { alert := btcwire.NewAlert( 1, 1337093712, 1368628812, 1015, 1013, []int32{1014}, 0, 40599, []string{"/Satoshi:0.7.2/"}, 5000, "", - "URGENT: upgrade required, see http://bitcoin.org/dos for details", "", + "URGENT: upgrade required, see http://bitcoin.org/dos for details", ) w := new(bytes.Buffer) err := alert.Serialize(w, pver) @@ -370,7 +370,7 @@ func TestAlertErrors(t *testing.T) { baseAlert := btcwire.NewAlert( 1, 1337093712, 1368628812, 1015, 1013, []int32{1014}, 0, 40599, []string{"/Satoshi:0.7.2/"}, 5000, "", - "URGENT", "", + "URGENT", ) baseAlertEncoded := []byte{ 0x01, 0x00, 0x00, 0x00, 0x50, 0x6e, 0xb2, 0x4f, 0x00, 0x00, 0x00, 0x00, 0x4c, 0x9e, 0x93, 0x51, //|....Pn.O....L..Q|