Sanitized vcedit_write() (and related) code some more.

diff --git a/ext/vcedit.c b/ext/vcedit.c
index f3ad2c95a1a1557468947739d46ec08838a70928..f5f493ae9203322130eb3ced4d346e456192518e 100644 (file)
--- a/ext/vcedit.c
+++ b/ext/vcedit.c
@@ -47,23 +47,47 @@ struct vcedit_state_St {
 
        bool opened;
        long serial;
 
        bool opened;
        long serial;

-       unsigned char *mainbuf;
-       unsigned char *bookbuf;
-       int     mainlen;
-       int     booklen;
+
+       ogg_packet packet_main;
+       ogg_packet packet_code_books;
+

        char *vendor;
        int prevW;
        char *vendor;
        int prevW;

-       int extrapage;
-       int eosin;
+
+       bool extra_page;
+       bool eos;

 
        char filename[0];
 };
 
 
        char filename[0];
 };
 

+static void
+ogg_packet_init (ogg_packet *p, unsigned char *buf, long len)
+{
+       p->packet = buf;
+       p->bytes = len;
+
+       p->b_o_s = p->e_o_s = 0;
+       p->granulepos = p->packetno = 0;
+}
+
+static bool
+ogg_packet_dup_data (ogg_packet *p)
+{
+       unsigned char *tmp;
+
+       tmp = malloc (p->bytes);
+       if (!tmp)
+               return false;
+
+       memcpy (tmp, p->packet, p->bytes);
+       p->packet = tmp;
+
+       return true;
+}
+

 static void
 vcedit_state_free (vcedit_state *s)
 {
 static void
 vcedit_state_free (vcedit_state *s)
 {

-       free (s->mainbuf);
-       free (s->bookbuf);

        free (s->vendor);
 
        if (s->in) {
        free (s->vendor);
 
        if (s->in) {


@@ -79,6 +103,9 @@ vcedit_state_init (vcedit_state *s, const char *filename)
 {
        s->refcount = 1;
 
 {
        s->refcount = 1;
 

+       ogg_packet_init (&s->packet_main, NULL, 0);
+       ogg_packet_init (&s->packet_code_books, NULL, 0);
+

        strcpy (s->filename, filename);
 
        return true;
        strcpy (s->filename, filename);
 
        return true;


@@ -126,13 +153,8 @@ vcedit_clear_internals (vcedit_state *s)
        free (s->vendor);
        s->vendor = NULL;
 
        free (s->vendor);
        s->vendor = NULL;
 

-       free (s->mainbuf);
-       s->mainbuf = NULL;
-       s->mainlen = 0;
-
-       free (s->bookbuf);
-       s->bookbuf = NULL;
-       s->booklen = 0;
+       ogg_packet_clear (&s->packet_main);
+       ogg_packet_clear (&s->packet_code_books);

 
        s->serial = 0;
        s->opened = false;
 
        s->serial = 0;
        s->opened = false;


@@ -166,13 +188,15 @@ _v_writestring (oggpack_buffer *o, char *s, int len)
                oggpack_write (o, *s++, 8);
 }
 
                oggpack_write (o, *s++, 8);
 }
 

-static int
-_commentheader_out (vcedit_state *s, ogg_packet *op)
+static bool
+write_comments (vcedit_state *s, ogg_packet *packet)

 {
        oggpack_buffer opb;
        size_t len;
        int i;
 
 {
        oggpack_buffer opb;
        size_t len;
        int i;
 

+       ogg_packet_init (packet, NULL, 0);
+

        oggpack_writeinit (&opb);
 
        /* preamble */
        oggpack_writeinit (&opb);
 
        /* preamble */


@@ -199,17 +223,17 @@ _commentheader_out (vcedit_state *s, ogg_packet *op)
 
        oggpack_write (&opb, 1, 1);
 
 
        oggpack_write (&opb, 1, 1);
 

-       op->bytes = oggpack_bytes (&opb);
-       op->packet = _ogg_malloc (op->bytes);
+       packet->bytes = oggpack_bytes (&opb);

 
 

-       memcpy (op->packet, opb.buffer, op->bytes);
+       packet->packet = _ogg_malloc (packet->bytes);
+       if (!packet->packet)
+               return false;

 
 

-       op->b_o_s = op->e_o_s = 0;
-       op->granulepos = 0;
+       memcpy (packet->packet, opb.buffer, packet->bytes);

 
        oggpack_writeclear (&opb);
 
 
        oggpack_writeclear (&opb);
 

-       return 0;
+       return true;

 }
 
 static int
 }
 
 static int


@@ -237,7 +261,7 @@ _fetch_next_packet (vcedit_state *s, ogg_packet *p, ogg_page *page)
        if (result == 1)
                return 1;
 
        if (result == 1)
                return 1;
 

-       if (s->eosin)
+       if (s->eos)

                return 0;
 
        while (ogg_sync_pageout (&s->oy, page) != 1) {
                return 0;
 
        while (ogg_sync_pageout (&s->oy, page) != 1) {


@@ -250,10 +274,11 @@ _fetch_next_packet (vcedit_state *s, ogg_packet *p, ogg_page *page)
        }
 
        if (ogg_page_eos (page))
        }
 
        if (ogg_page_eos (page))

-               s->eosin = 1;
+               s->eos = true;

        else if (ogg_page_serialno (page) != s->serial) {
        else if (ogg_page_serialno (page) != s->serial) {

-               s->eosin = 1;
-               s->extrapage = 1;
+               s->eos = true;
+               s->extra_page = true;
+

                return 0;
        }
 
                return 0;
        }
 


@@ -266,9 +291,8 @@ vcedit_error
 vcedit_open (vcedit_state *s)
 {
        vcedit_error ret;
 vcedit_open (vcedit_state *s)
 {
        vcedit_error ret;

-       ogg_packet *header;
-       ogg_packet header_main, header_comments, header_codebooks;
-       ogg_page og;
+       ogg_packet packet_comments, *header = &packet_comments;
+       ogg_page page;

        struct stat st;
        char *buffer;
        size_t bytes, total = 0;
        struct stat st;
        char *buffer;
        size_t bytes, total = 0;


@@ -296,34 +320,36 @@ vcedit_open (vcedit_state *s)
                total += bytes;
 
                ogg_sync_wrote (&s->oy, bytes);
                total += bytes;
 
                ogg_sync_wrote (&s->oy, bytes);

-       } while (ogg_sync_pageout (&s->oy, &og) != 1);
+       } while (ogg_sync_pageout (&s->oy, &page) != 1);

 
 

-       s->serial = ogg_page_serialno (&og);
+       s->serial = ogg_page_serialno (&page);

 
        ogg_stream_init (&s->os, s->serial);
        vorbis_info_init (&s->vi);
        vorbis_comment_init (&s->vc);
 
 
        ogg_stream_init (&s->os, s->serial);
        vorbis_info_init (&s->vi);
        vorbis_comment_init (&s->vc);
 

-       if (ogg_stream_pagein (&s->os, &og) < 0) {
+       if (ogg_stream_pagein (&s->os, &page) < 0) {

                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 
                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 

-       if (ogg_stream_packetout (&s->os, &header_main) != 1) {
+       if (ogg_stream_packetout (&s->os, &s->packet_main) != 1) {

                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 
                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 

-       if (vorbis_synthesis_headerin (&s->vi, &s->vc, &header_main) < 0) {
+       if (!ogg_packet_dup_data (&s->packet_main)) {
+               s->packet_main.packet = NULL;

                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 
                ret = VCEDIT_ERR_INVAL;
                goto err;
        }
 

-       s->mainlen = header_main.bytes;
-       s->mainbuf = malloc (s->mainlen);
-       memcpy (s->mainbuf, header_main.packet, header_main.bytes);
+       if (vorbis_synthesis_headerin (&s->vi, &s->vc, &s->packet_main) < 0) {
+               ret = VCEDIT_ERR_INVAL;
+               goto err;
+       }

 
 

-       header = &header_comments;
+       ogg_packet_init (&packet_comments, NULL, 0);

 
        while (i < 2) {
                if (feof (s->in) || ferror (s->in)) {
 
        while (i < 2) {
                if (feof (s->in) || ferror (s->in)) {


@@ -334,14 +360,14 @@ vcedit_open (vcedit_state *s)
                while (i < 2) {
                        int result;
 
                while (i < 2) {
                        int result;
 

-                       result = ogg_sync_pageout (&s->oy, &og);
+                       result = ogg_sync_pageout (&s->oy, &page);

                        if (!result)
                                break; /* Too little data so far */
 
                        if (result != 1)
                                continue;
 
                        if (!result)
                                break; /* Too little data so far */
 
                        if (result != 1)
                                continue;
 

-                       ogg_stream_pagein (&s->os, &og);
+                       ogg_stream_pagein (&s->os, &page);

 
                        while (i < 2) {
                                result = ogg_stream_packetout (&s->os, header);
 
                        while (i < 2) {
                                result = ogg_stream_packetout (&s->os, header);


@@ -353,15 +379,15 @@ vcedit_open (vcedit_state *s)
                                        goto err;
                                }
 
                                        goto err;
                                }
 

-                               vorbis_synthesis_headerin (&s->vi, &s->vc, header);
-
-                               if (i++ == 1) {
-                                       s->booklen = header->bytes;
-                                       s->bookbuf = malloc (s->booklen);
-                                       memcpy (s->bookbuf, header->packet, header->bytes);
+                               if (i++ == 1 && !ogg_packet_dup_data (header)) {
+                                       header->packet = NULL;
+                                       ret = VCEDIT_ERR_INVAL;
+                                       goto err;

                                }
 
                                }
 

-                               header = &header_codebooks;
+                               vorbis_synthesis_headerin (&s->vi, &s->vc, header);
+
+                               header = &s->packet_code_books;

                        }
                }
 
                        }
                }
 


@@ -385,7 +411,7 @@ err:
        return ret;
 }
 
        return ret;
 }
 

-static int
+static bool

 write_data (const void *buf, size_t size, size_t nmemb, FILE *stream)
 {
        while (nmemb > 0) {
 write_data (const void *buf, size_t size, size_t nmemb, FILE *stream)
 {
        while (nmemb > 0) {


@@ -393,26 +419,33 @@ write_data (const void *buf, size_t size, size_t nmemb, FILE *stream)
 
                w = fwrite (buf, size, nmemb, stream);
                if (!w && ferror (stream))
 
                w = fwrite (buf, size, nmemb, stream);
                if (!w && ferror (stream))

-                       return 0;
+                       return false;

 
                nmemb -= w;
                buf += size * w;
        }
 
 
                nmemb -= w;
                buf += size * w;
        }
 

-       return 1;
+       return true;
+}
+
+static bool
+write_page (FILE *f, ogg_page *p)
+{
+       return write_data (p->header, 1, p->header_len, f) &&
+              write_data (p->body, 1, p->body_len, f);

 }
 
 vcedit_error
 vcedit_write (vcedit_state *s)
 {
 }
 
 vcedit_error
 vcedit_write (vcedit_state *s)
 {

-       ogg_stream_state streamout;
-       ogg_packet header_main, header_comments, header_codebooks, op;
-       ogg_page ogout, ogin;
+       ogg_stream_state stream;
+       ogg_packet packet;
+       ogg_page page_out, page_in;

        ogg_int64_t granpos = 0;
        FILE *out;
        char *buffer, tmpfile[PATH_MAX];
        ogg_int64_t granpos = 0;
        FILE *out;
        char *buffer, tmpfile[PATH_MAX];

-       bool success = false;
-       int fd, result, bytes, needflush = 0, needout = 0;
+       bool success = false, need_flush = false, need_out = false;
+       int fd, result, bytes;

 
        if (!s->opened)
                return VCEDIT_ERR_INVAL;
 
        if (!s->opened)
                return VCEDIT_ERR_INVAL;


@@ -432,116 +465,92 @@ vcedit_write (vcedit_state *s)
                return VCEDIT_ERR_TMPFILE;
        }
 
                return VCEDIT_ERR_TMPFILE;
        }
 

-       s->prevW = s->extrapage = s->eosin = 0;
+       s->prevW = 0;
+       s->extra_page = s->eos = false;

 
 

-       header_main.bytes = s->mainlen;
-       header_main.packet = s->mainbuf;
-       header_main.b_o_s = 1;
-       header_main.e_o_s = 0;
-       header_main.granulepos = 0;
+       ogg_stream_init (&stream, s->serial);

 
 

-       header_codebooks.bytes = s->booklen;
-       header_codebooks.packet = s->bookbuf;
-       header_codebooks.b_o_s = 0;
-       header_codebooks.e_o_s = 0;
-       header_codebooks.granulepos = 0;
+       /* write "main" packet */
+       s->packet_main.b_o_s = 1;
+       ogg_stream_packetin (&stream, &s->packet_main);
+       s->packet_main.b_o_s = 0;

 
 

-       ogg_stream_init (&streamout, s->serial);
+       /* prepare and write comments */
+       if (!write_comments (s, &packet)) {
+               ogg_stream_clear (&stream);
+               unlink (tmpfile);
+               fclose (out);

 
 

-       _commentheader_out (s, &header_comments);
+               return VCEDIT_ERR_INVAL;
+       }

 
 

-       ogg_stream_packetin (&streamout, &header_main);
-       ogg_stream_packetin (&streamout, &header_comments);
-       ogg_stream_packetin (&streamout, &header_codebooks);
+       ogg_stream_packetin (&stream, &packet);
+       ogg_packet_clear (&packet);

 
 

-       while ((result = ogg_stream_flush (&streamout, &ogout))) {
-               if (!write_data (ogout.header, 1, ogout.header_len, out))
-                       goto cleanup;
+       /* write codebooks */
+       ogg_stream_packetin (&stream, &s->packet_code_books);

 
 

-               if (!write_data (ogout.body, 1, ogout.body_len, out))
+       while (ogg_stream_flush (&stream, &page_out))
+               if (!write_page (out, &page_out))

                        goto cleanup;
                        goto cleanup;

-       }

 
 

-       while (_fetch_next_packet (s, &op, &ogin)) {
+       while (_fetch_next_packet (s, &packet, &page_in)) {
+               bool write = false;

                int size;
 
                int size;
 

-               size = _blocksize (s, &op);
+               size = _blocksize (s, &packet);

                granpos += size;
 
                granpos += size;
 

-               if (needflush) {
-                       if (ogg_stream_flush (&streamout, &ogout)) {
-                               if (!write_data (ogout.header, 1, ogout.header_len, out))
-                                       goto cleanup;
-
-                               if (!write_data (ogout.body, 1, ogout.body_len, out))
-                                       goto cleanup;
-                       }
-               } else if (needout) {
-                       if (ogg_stream_pageout (&streamout, &ogout)) {
-                               if (!write_data (ogout.header, 1, ogout.header_len, out))
-                                       goto cleanup;
-
-                               if (!write_data (ogout.body, 1, ogout.body_len, out))
-                                       goto cleanup;
-                       }
+               if (need_flush) {
+                       write = ogg_stream_flush (&stream, &page_out);
+               } else if (need_out) {
+                       write = ogg_stream_pageout (&stream, &page_out);

                }
 
                }
 

-               needflush = needout = 0;
+               if (write && !write_page (out, &page_out))
+                       goto cleanup;
+
+               need_flush = need_out = false;

 
 

-               if (op.granulepos == -1) {
-                       op.granulepos = granpos;
-                       ogg_stream_packetin (&streamout, &op);
+               if (packet.granulepos == -1) {
+                       packet.granulepos = granpos;
+                       ogg_stream_packetin (&stream, &packet);

                } else {
                        /* granulepos is set, validly. Use it, and force a flush to
                         * account for shortened blocks (vcut) when appropriate
                         */
                } else {
                        /* granulepos is set, validly. Use it, and force a flush to
                         * account for shortened blocks (vcut) when appropriate
                         */

-                       if (granpos > op.granulepos) {
-                               granpos = op.granulepos;
-                               ogg_stream_packetin (&streamout, &op);
-                               needflush = 1;
+                       if (granpos > packet.granulepos) {
+                               granpos = packet.granulepos;
+                               ogg_stream_packetin (&stream, &packet);
+                               need_flush = true;

                        } else {
                        } else {

-                               ogg_stream_packetin (&streamout, &op);
-                               needout = 1;
+                               ogg_stream_packetin (&stream, &packet);
+                               need_out = true;

                        }
                }
        }
 
                        }
                }
        }
 

-       streamout.e_o_s = 1;
+       stream.e_o_s = 1;

 
 

-       while (ogg_stream_flush (&streamout, &ogout)) {
-               if (!write_data (ogout.header, 1, ogout.header_len, out))
+       while (ogg_stream_flush (&stream, &page_out))
+               if (!write_page (out, &page_out))

                        goto cleanup;
 
                        goto cleanup;
 

-               if (!write_data (ogout.body, 1, ogout.body_len, out))
-                       goto cleanup;
-       }
-
-       if (s->extrapage) {
-               if (!write_data (ogin.header, 1, ogin.header_len, out))
-                       goto cleanup;
-
-               if (!write_data (ogin.body, 1, ogin.body_len, out))
-                       goto cleanup;
-       }
+       if (s->extra_page && !write_page (out, &page_in))
+               goto cleanup;

 
        /* clear it, because not all paths to here do */
 
        /* clear it, because not all paths to here do */

-       s->eosin = 0;
+       s->eos = false;

 
        do {
                /* We copy the rest of the stream (other logical streams)
                 * through, a page at a time.
                 */
 
        do {
                /* We copy the rest of the stream (other logical streams)
                 * through, a page at a time.
                 */

-               while ((result = ogg_sync_pageout (&s->oy, &ogout))) {
-                       if (result != 1)
-                               continue;
-
+               while ((result = ogg_sync_pageout (&s->oy, &page_out))) {

                        /* Don't bother going through the rest, we can just
                         * write the page out now
                         */
                        /* Don't bother going through the rest, we can just
                         * write the page out now
                         */

-                       if (!write_data (ogout.header, 1, ogout.header_len, out))
-                               goto cleanup;
-
-                       if (!write_data (ogout.body, 1, ogout.body_len, out))
+                       if (result == 1 && !write_page (out, &page_out))

                                goto cleanup;
                }
 
                                goto cleanup;
                }
 


@@ -551,11 +560,9 @@ vcedit_write (vcedit_state *s)
 
                if (ferror (s->in))
                        goto cleanup;
 
                if (ferror (s->in))
                        goto cleanup;

+       } while (bytes || !feof (s->in));

 
 

-               s->eosin = !bytes && feof (s->in);
-       } while (!s->eosin);
-
-       success = true;
+       s->eos = success = true;

 
 cleanup:
        fclose (s->in);
 
 cleanup:
        fclose (s->in);


@@ -570,19 +577,9 @@ cleanup:
                chmod (s->filename, s->file_mode);
        }
 
                chmod (s->filename, s->file_mode);
        }
 

-       ogg_stream_clear (&streamout);
-
-    /* We don't ogg_packet_clear() this, because the memory was
-        * allocated in _commentheader_out(), so we mirror that here
-        */
-    _ogg_free (header_comments.packet);
-
-       free (s->mainbuf);
-       free (s->bookbuf);
-
-    s->mainbuf = s->bookbuf = NULL;
+       ogg_stream_clear (&stream);

 
 

-       if (!s->eosin)
+       if (!s->eos)

                return VCEDIT_ERR_INVAL;
 
        vcedit_clear_internals (s);
                return VCEDIT_ERR_INVAL;
 
        vcedit_clear_internals (s);