Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevents memory underflow in GFileMimeType() in gutils/fsys.c #5018

Merged
merged 1 commit into from Jul 3, 2022

Conversation

Omnikron13
Copy link
Contributor

Short-circuits out the function in the specific case of '..', which was under-flowing the later check if (pt[len - 1] == '~')

Type of change

  • Bug fix
  • Non-breaking change

@frank-trampe
Copy link
Contributor

Are you sure that's where it's crashing? strrchr gets the last occurrence, so it seems to me that pt - 1 would be valid even if len is 0 and path is "..". Either way, we ought to implement formal bounds-checking if this is a problem in addition to any special handling. In particular, if ".." crashes it, "." or ".a" probably does too.

@skef
Copy link
Contributor

skef commented May 20, 2022

"." or ".a" probably does too.

Or "a.".

@Omnikron13 I think the easy fix here is to remove what you added and then below at the start of the else change

            pt = copy(pt + 1);
            int len = strlen(pt);

to something like

            int len = strlen(pt);
            if (len <= 1)
                return copy("application/octet-stream")
            else
                 len--
            pt = copy(pt + 1);

@Omnikron13
Copy link
Contributor Author

Omnikron13 commented May 21, 2022

Are you sure that's where it's crashing? strrchr gets the last occurrence, so it seems to me that pt - 1 would be valid even if len is 0 and path is "..". Either way, we ought to implement formal bounds-checking if this is a problem in addition to any special handling. In particular, if ".." crashes it, "." or ".a" probably does too.

pt = strrchr(path, '.'); will index the last actual character before the null terminator, pt = copy(pt + 1); moves the pointer onto the null terminator, and finally pt[len - 1], which is equivalent to pt[0 - 1] will underflow.

Just . doesn't actually appear to occur; I guess it is filtered out somewhere else before getting here?

Files with single character extensions like .c or .h files are fine as strrchr() only causes a problem with .. because it takes the right-most character.

I believe that just this:

if(len < 1)
    return copy("inode/directory");

Should be sufficient to handle it that way. Is there a particular reason to return application/octet-stream over inode/directory?

The only concern I can think of is that this would misidentify any file that ends with just . as a directory, though I'm not sure how likely that eventuality is vs. it being .. that is detected.

@jtanx jtanx closed this Jul 3, 2022
@jtanx jtanx reopened this Jul 3, 2022
gutils/fsys.c Outdated
@@ -1039,6 +1039,11 @@ char* GFileMimeType(const char *path) {
} else {
pt = copy(pt + 1);
int len = strlen(pt);

// Handle the .. 'directory'. Prevents underflowing below
if(len < 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is correct, what if you had a file named foo..

Can you just change the below check to if(len > 0 && pt[len - 1] == '~')

Copy link
Contributor

@jtanx jtanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

@jtanx jtanx merged commit 05c02ab into fontforge:master Jul 3, 2022
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.

None yet

4 participants