Skip to content

fix: Out-of-bounds memory access in strtok.#192

Merged
ooxi merged 1 commit intogerbv:mainfrom
eyal0:parse_aperture_strtok
Jul 13, 2023
Merged

fix: Out-of-bounds memory access in strtok.#192
ooxi merged 1 commit intogerbv:mainfrom
eyal0:parse_aperture_strtok

Conversation

@eyal0
Copy link
Copy Markdown
Collaborator

@eyal0 eyal0 commented Jul 12, 2023 •
edited
Loading

This fixes #191

@eyal0 eyal0 marked this pull request as draft July 12, 2023 18:47
@eyal0
Copy link
Copy Markdown
Collaborator Author

eyal0 commented Jul 12, 2023

This is currently a draft. It has a unit test but no fix yet.

@ooxi
Copy link
Copy Markdown
Contributor

ooxi commented Jul 13, 2023

Thank you for the testcase! I think the best way forward is to set a default value for fd->filename in gerb_fopen. I was considering strduping the filename argument, but that might lead to even more memory leaks.

Thus I'll try setting it to nullptr first.

@ooxi
Copy link
Copy Markdown
Contributor

ooxi commented Jul 13, 2023

I have tried both nullptr and "" as default initialization. In both cases the Conditional jump or move depends on uninitialised value message goes away, but valgrind does not pass, either.

However, I cannot make out the cause of the error. Do you see an error: https://github.com/ooxi/gerbv/actions/runs/5545198229/jobs/10123767309?

@eyal0 eyal0 force-pushed the parse_aperture_strtok branch from 3af7a07 to b3ae98e Compare July 13, 2023 17:14
@eyal0
fix: Out-of-bounds memory access of filename.
dfb5aac 
Put all the filename allocation and deallocation into the `gerb_fopen`
and `gerb_fclose` functions so that the caller doesn't need to deal
with this anymore.

Also, free includeFilename.  It was allocated as part of
`gerb_fgetstring()` above but never freed properly.

Unit tests added to valgrind.
@eyal0 eyal0 force-pushed the parse_aperture_strtok branch from b3ae98e to dfb5aac Compare July 13, 2023 17:28
@eyal0
Copy link
Copy Markdown
Collaborator Author

eyal0 commented Jul 13, 2023 

==32056== 28 bytes in 1 blocks are definitely lost in loss record 110 of 296
==32056==    at 0x483C855: malloc (vg_replace_malloc.c:393)
==32056==    by 0x506BE98: g_malloc (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6)
==32056==    by 0x4865BD6: gerb_fgetstring (gerb_file.c:219)
==32056==    by 0x486BFC1: parse_rs274x (gerber.c:1450)
==32056==    by 0x486A68F: gerber_parse_file_segment (gerber.c:245)
==32056==    by 0x486E507: parse_gerb (gerber.c:770)
==32056==    by 0x486F148: gerbv_open_image (gerbv.c:571)
==32056==    by 0x486F59B: gerbv_open_layer_from_filename_with_color (gerbv.c:249)
==32056==    by 0x11B452: main (main.c:941)

@ooxi Setting it to nullptr or "" fixed the problem of trying to read bad memory. However, it exposed a different problem, which I also encountered, same as you! It's saying that there is memory that was allocated but never freed, and it shows the stack trace to that allocation. Along that path, I can see parse_rs274x (gerber.c:1450). In the code, it's here:

gerbv/src/gerber.c

Lines 1450 to 1480 in caa6560

gchar *includeFilename = gerb_fgetstring(fd, '*');
if (includeFilename) {
gchar *fullPath;
if (!g_path_is_absolute(includeFilename)) {
fullPath = g_build_filename (directoryPath, includeFilename, NULL);
} else {
fullPath = g_strdup (includeFilename);
}
if (levelOfRecursion < 10) {
gerb_file_t *includefd = NULL;
includefd = gerb_fopen(fullPath);
if (includefd) {
gerber_parse_file_segment (levelOfRecursion + 1, image, state, curr_net, stats, includefd, directoryPath);
gerb_fclose(includefd);
} else {
gerbv_stats_printf(error_list, GERBV_MESSAGE_ERROR, -1,
_("Included file \"%s\" cannot be found "
"at line %ld in file \"%s\""),
fullPath, *line_num_p, fd->filename);
}
g_free (fullPath);
} else {
gerbv_stats_printf(error_list, GERBV_MESSAGE_ERROR, -1,
_("Parser encountered more than 10 levels of "
"include file recursion which is not allowed "
"by the RS-274X spec"));
}
}

Looking into the code for that call to gerb_fgetstring, I see:

gerbv/src/gerb_file.c

Lines 196 to 224 in caa6560

gerb_fgetstring(gerb_file_t *fd, char term)
{
char *strend = NULL;
char *newstr;
char *i, *iend;
int len;
iend = fd->data + fd->datalen;
for (i = fd->data + fd->ptr; i < iend; i++) {
if (*i == term) {
strend = i;
break;
}
}
if (strend == NULL)
return NULL;
len = strend - (fd->data + fd->ptr);
newstr = (char *)g_malloc(len + 1);
if (newstr == NULL)
return NULL;
strncpy(newstr, fd->data + fd->ptr, len);
newstr[len] = '\0';
fd->ptr += len;
return newstr;
} /* gerb_fgetstring */

There's a g_malloc in there with no corresponding free. So it needs to be free'd! We've never caught this before in valgrind because we have never covered this part of the code with valgrind. :-( So that's where the memory is getting lost. You can see that the size lost is 28 bytes and the file name that @iosifache chose was parse_aperture_strtok.2.poc and that's 27 letters. Add one for the terminating null and it's 28, same as the malloc size.

@eyal0 eyal0 marked this pull request as ready for review July 13, 2023 17:29
ooxi
ooxi approved these changes Jul 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ooxi ooxi left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@ooxi ooxi merged commit 5517e22 into gerbv:main Jul 13, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ooxi ooxi ooxi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Denial of Service in Gerbv

2 participants

@eyal0 @ooxi