Merge pull request #4792 from ipfs/fix/read-eof

fix incorrect Read calls

cmd/seccat/util.go

@@ -31,7 +31,7 @@ type logRW struct {
 
 func (r *logRW) Read(buf []byte) (int, error) {
 	n, err := r.rw.Read(buf)
-	if err == nil {
+	if n > 0 {
 		log.Debugf("%s read: %v", r.n, buf)
 	}
 	return n, err

repo/fsrepo/fsrepo.go

@@ -324,13 +324,21 @@ func APIAddr(repoPath string) (ma.Multiaddr, error) {
 
 	// read up to 2048 bytes. io.ReadAll is a vulnerability, as
 	// someone could hose the process by putting a massive file there.
-	buf := make([]byte, 2048)
-	n, err := f.Read(buf)
-	if err != nil && err != io.EOF {
+	//
+	// NOTE(@stebalien): @jbenet probably wasn't thinking straight when he
+	// wrote that comment but I'm leaving the limit here in case there was
+	// some hidden wisdom. However, I'm fixing it such that:
+	// 1. We don't read too little.
+	// 2. We don't truncate and succeed.
+	buf, err := ioutil.ReadAll(io.LimitReader(f, 2048))
+	if err != nil {
 		return nil, err
 	}
+	if len(buf) == 2048 {
+		return nil, fmt.Errorf("API file too large, must be <2048 bytes long: %s", apiFilePath)
+	}
 
-	s := string(buf[:n])
+	s := string(buf)
 	s = strings.TrimSpace(s)
 	return ma.NewMultiaddr(s)
 }

unixfs/io/pbdagreader.go

@@ -166,22 +166,20 @@ func (dr *PBDagReader) CtxReadFull(ctx context.Context, b []byte) (int, error) {
 	total := 0
 	for {
 		// Attempt to fill bytes from cached buffer
-		n, err := dr.buf.Read(b[total:])
+		n, err := io.ReadFull(dr.buf, b[total:])
 		total += n
 		dr.offset += int64(n)
-		if err != nil {
-			// EOF is expected
-			if err != io.EOF {
-				return total, err
-			}
-		}
-
-		// If weve read enough bytes, return
-		if total == len(b) {
+		switch err {
+		// io.EOF will happen is dr.buf had noting more to read (n == 0)
+		case io.EOF, io.ErrUnexpectedEOF:
+			// do nothing
+		case nil:
 			return total, nil
+		default:
+			return total, err
 		}
 
-		// Otherwise, load up the next block
+		// if we are not done with the output buffer load next block
 		err = dr.precalcNextBuf(ctx)
 		if err != nil {
 			return total, err

unixfs/mod/dagmodifier.go

@@ -202,7 +202,7 @@ func (dm *DagModifier) Sync() error {
 	buflen := dm.wrBuf.Len()
 
 	// overwrite existing dag nodes
-	thisc, done, err := dm.modifyDag(dm.curNode, dm.writeStart, dm.wrBuf)
+	thisc, err := dm.modifyDag(dm.curNode, dm.writeStart)
 	if err != nil {
 		return err
 	}
@@ -213,7 +213,7 @@ func (dm *DagModifier) Sync() error {
 	}
 
 	// need to write past end of current dag
-	if !done {
+	if dm.wrBuf.Len() > 0 {
 		dm.curNode, err = dm.appendData(dm.curNode, dm.splitter(dm.wrBuf))
 		if err != nil {
 			return err
@@ -231,28 +231,27 @@ func (dm *DagModifier) Sync() error {
 	return nil
 }
 
-// modifyDag writes the data in 'data' over the data in 'node' starting at 'offset'
-// returns the new key of the passed in node and whether or not all the data in the reader
-// has been consumed.
-func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*cid.Cid, bool, error) {
+// modifyDag writes the data in 'dm.wrBuf' over the data in 'node' starting at 'offset'
+// returns the new key of the passed in node.
+func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64) (*cid.Cid, error) {
 	// If we've reached a leaf node.
 	if len(n.Links()) == 0 {
 		switch nd0 := n.(type) {
 		case *mdag.ProtoNode:
 			f, err := ft.FromBytes(nd0.Data())
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 
-			n, err := data.Read(f.Data[offset:])
+			_, err = dm.wrBuf.Read(f.Data[offset:])
 			if err != nil && err != io.EOF {
-				return nil, false, err
+				return nil, err
 			}
 
 			// Update newly written node..
 			b, err := proto.Marshal(f)
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 
 			nd := new(mdag.ProtoNode)
@@ -260,16 +259,10 @@ func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*c
 			nd.SetPrefix(&nd0.Prefix)
 			err = dm.dagserv.Add(dm.ctx, nd)
 			if err != nil {
-				return nil, false, err
-			}
-
-			// Hey look! we're done!
-			var done bool
-			if n < len(f.Data[offset:]) {
-				done = true
+				return nil, err
 			}
 
-			return nd.Cid(), done, nil
+			return nd.Cid(), nil
 		case *mdag.RawNode:
 			origData := nd0.RawData()
 			bytes := make([]byte, len(origData))
@@ -278,9 +271,9 @@ func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*c
 			copy(bytes, origData[:offset])
 
 			// copy in new data
-			n, err := data.Read(bytes[offset:])
+			n, err := dm.wrBuf.Read(bytes[offset:])
 			if err != nil && err != io.EOF {
-				return nil, false, err
+				return nil, err
 			}
 
 			// copy remaining data
@@ -291,46 +284,39 @@ func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*c
 
 			nd, err := mdag.NewRawNodeWPrefix(bytes, nd0.Cid().Prefix())
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 			err = dm.dagserv.Add(dm.ctx, nd)
 			if err != nil {
-				return nil, false, err
-			}
-
-			// Hey look! we're done!
-			var done bool
-			if n < len(bytes[offset:]) {
-				done = true
+				return nil, err
 			}
 
-			return nd.Cid(), done, nil
+			return nd.Cid(), nil
 		}
 	}
 
 	node, ok := n.(*mdag.ProtoNode)
 	if !ok {
-		return nil, false, ErrNotUnixfs
+		return nil, ErrNotUnixfs
 	}
 
 	f, err := ft.FromBytes(node.Data())
 	if err != nil {
-		return nil, false, err
+		return nil, err
 	}
 
 	var cur uint64
-	var done bool
 	for i, bs := range f.GetBlocksizes() {
 		// We found the correct child to write into
 		if cur+bs > offset {
 			child, err := node.Links()[i].GetNode(dm.ctx, dm.dagserv)
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 
-			k, sdone, err := dm.modifyDag(child, offset-cur, data)
+			k, err := dm.modifyDag(child, offset-cur)
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 
 			node.Links()[i].Cid = k
@@ -338,12 +324,11 @@ func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*c
 			// Recache serialized node
 			_, err = node.EncodeProtobuf(true)
 			if err != nil {
-				return nil, false, err
+				return nil, err
 			}
 
-			if sdone {
+			if dm.wrBuf.Len() == 0 {
 				// No more bytes to write!
-				done = true
 				break
 			}
 			offset = cur + bs
@@ -352,7 +337,7 @@ func (dm *DagModifier) modifyDag(n ipld.Node, offset uint64, data io.Reader) (*c
 	}
 
 	err = dm.dagserv.Add(dm.ctx, node)
-	return node.Cid(), done, err
+	return node.Cid(), err
 }
 
 // appendData appends the blocks from the given chan to the end of this dag