Factor out get_wwwroot_parent and test it. #65
Conversation
|
@g-rden what do you think? |
| } | ||
|
|
||
| int main(void) { | ||
| test("", ".", ""); |
There was a problem hiding this comment.
This is a case that can't occur in darkhttpd. wwwroot can't be NULL at this point and
path[0] = '.';
path[1] = '\0';
in get_wwwroot_parent() would write to unallocated memory. So this test is not necessary I think.
These tests would be good too I think. Just to cover all bases.
test("x", ".", "x"); // 1 char
test(".dir/file", ".dir/", "/file"); // hidden dir
test(".file", ".", ".file"); // hidden file
test("/file", "/", "/file"); // file in root
| static void xchroot(const char *path) { | ||
| if (chdir(path) == -1) | ||
| err(1, "chdir(%s)", path); | ||
| if (chroot(path) == -1) |
There was a problem hiding this comment.
This should be if (chroot(".") == -1)
This is my bad. I tested with absolute paths, so I didn't notice that we already are in the directory we want to chroot to. I think we can leave path in the error messages.
| char* get_wwwroot_parent() { | ||
| off_t ofs; | ||
| size_t len = strlen(wwwroot) + 1; | ||
| if (len == 1) return xstrdup("."); |
There was a problem hiding this comment.
len can't be 1. It's the length of wwwroot including NULL terminator.
We could remove + 1, but then memmove(wwwroot, &wwwroot[ofs], len + 1 - ofs); could be a bit more confusing.
I don't mind either way.
I am dumb, just do len == 2
| char* get_wwwroot_parent() { | ||
| off_t ofs; | ||
| size_t len = strlen(wwwroot) + 1; |
There was a problem hiding this comment.
The last things I would change:
static char* get_wwwroot_parent(void) {
and something I forgot again
const size_t len = strlen(wwwroot) + 1;
While being on the topic of pedantic changes, I noticed a few things in darkhttpd.c, none of them matter much, so I don't know if I want to make a PR, but if you want I can make it.
| } | ||
|
|
||
| int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { | ||
| wwwroot = sdup(data, size); |
There was a problem hiding this comment.
It test for data = NULL. This is the same problem we have with test("", ".", "");. We can account for wwwroot being NULL in get_wwwroot_parent(), but like I said, I don't think it is worth it.
I couldn't find anything to disallow the fuzzer from testing a string or a string length. So I added if (size == 0) return 0; to run it.
| return 0; | ||
| } | ||
|
|
||
| /* vim:set tabstop=4 shiftwidth=4 expandtab tw=78: */ |
There was a problem hiding this comment.
Might it be worth setting these settings in the .editorconfig?
|
I'll get back to this eventually. :) Thanks for the comments. |
|
Hey @emikulic! Might you have time to take a look at this again? I might end up writing another PR, but didn't want to introduce any merge conflicts. Thanks! |
|
Go ahead without me. I'll get back to this eventually. (I want to do it in a different way anyway) |
No description provided.