Skip to content

epub: avoid a fixed-size buffer when copying the content path prefix#715

Open
maxheise wants to merge 1 commit into
linuxmint:masterfrom
maxheise:epub-content-path-buffer
Open

epub: avoid a fixed-size buffer when copying the content path prefix#715
maxheise wants to merge 1 commit into
linuxmint:masterfrom
maxheise:epub-content-path-buffer

Conversation

@maxheise
Copy link
Copy Markdown

Hi, and thanks for xreader.

I have been writing a markdown backend for xreader as a hobby, with help
from an AI coding assistant, which flagged this while I was in the EPUB
code. I reviewed it and think it is a small, worthwhile cleanup.

In get_uri_to_content() (backend/epub/epub-document.c), the directory
part of the OPF path is copied into a fixed buffer with a hand-written
loop that runs until the last /, with no bound on how much it copies:

gchar* directorybuffer = g_malloc0(sizeof(gchar*)*100);
gchar* writer = directorybuffer;
while (copybuffer != documentfolder) {
        (*writer) = (*copybuffer);
        writer++; copybuffer++;
}
*writer = '\0';

The string being copied is relativepath, the full-path attribute read
from the EPUB's META-INF/container.xml, so its length is whatever the
file declares; nothing here bounds it.

Two things about the allocation also stand out:

  • The element size is sizeof(gchar*), the size of a pointer (8 bytes
    on 64-bit). But the buffer holds a string of gchar, i.e. characters,
    not pointers. The size that matches the data being copied is
    sizeof(gchar) (1 byte), or simply a byte count. So the * is the
    wrong type for what is actually stored, and the buffer ends up ~800
    bytes only by accident.
  • 100 is a magic number: there is no explanation for why it is 100.

Because the copy loop is unbounded, the buffer's correctness rests on
those accidents. Normal EPUBs use short OPF paths, so this is not hit in
everyday use, but since the length comes from the file, a long enough
full-path would copy past the buffer. And if someone corrected the
obvious type mismatch, sizeof(gchar*) to sizeof(gchar), the buffer
would shrink to 100 bytes and the same loop would write past the end on
ordinary paths.

The fix computes the length and uses g_strndup():

gsize dir_len = documentfolder - (gchar *) relativepath;
gchar *directorybuffer = g_strndup ((gchar *) relativepath, dir_len);

g_strndup allocates a buffer sized to the prefix length, so the copy
stays within bounds regardless of the input. It also removes the magic
number, the wrong-typed sizeof, and the hand-written loop, and produces
the same string as before.

Thanks for taking a look.

In get_uri_to_content() the directory part of the OPF path is copied
into a fixed buffer with a hand-written loop that runs until the last
'/', with no bound on how much it copies.

The string copied is relativepath, the full-path attribute read from
the EPUB's META-INF/container.xml, so its length is whatever the file
declares. Two things about the allocation also stand out. The element
size is sizeof(gchar*), the size of a pointer (8 bytes on 64-bit), but
the buffer holds a string of gchar, i.e. characters, not pointers; the
size that matches the data is sizeof(gchar) (1 byte) or simply a byte
count, so the '*' is the wrong type for what is stored and the buffer
is ~800 bytes only by accident. And 100 is a magic number with no
explanation for why it is 100.

Because the copy loop is unbounded, the buffer's correctness rests on
those accidents. Normal EPUBs use short OPF paths and are unaffected,
but a file with a long enough full-path would copy past the buffer.
And if someone corrected the type mismatch, sizeof(gchar*) to
sizeof(gchar), the buffer would shrink to 100 bytes and the same loop
would write past the end on ordinary paths.

Compute the prefix length as the distance to the last '/' and use
g_strndup(), which allocates a buffer sized to the data and
NUL-terminates it, so the copy stays in bounds. This also removes the
magic number, the wrong-typed sizeof, and the hand-written loop, and
produces the same string. The freeing of the result is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant