klange / toaruos

A completely-from-scratch hobby operating system: bootloader, kernel, drivers, C library, and userspace including a composited graphical UI, dynamic linker, syntax-highlighting text editor, network stack, etc.
https://toaruos.org/
University of Illinois/NCSA Open Source License
6.03k stars 475 forks source link

imgviewer segmentation fault #188

Closed ZanyXDev closed 4 years ago

ZanyXDev commented 4 years ago

If run imgviewer without arguments - result segmentation fault. ToaruOS version 1.10.11

ZanyXDev commented 4 years ago

The problem is that the sprite download function in the file lib/graphics.c void load_sprite(sprite_t sprite, char filename) does not check for the existence of the file.

ZanyXDev commented 4 years ago

Standard POSIX Standard: 2.10 Symbolic Constants file include: / Values for the second argument to access. These may be OR'd together. /

define R_OK 4 / Test for read permission. /

define W_OK 2 / Test for write permission. /

define X_OK 1 / Test for execute permission. /

define F_OK 0 / Test for existence. /

/ Test for access to NAME using the real UID and real GID. / extern int access (const char *name, int type) THROW nonnull ((1)); But file toaruos/base/usr/include/unistd.h not this access

klange commented 4 years ago

unistd.h does define access

The _OK constants are, perhaps erroneously, defined in fcntl.h

But the underlying system call only checks if the file can be opened.

ZanyXDev commented 4 years ago

Thanks

ZanyXDev commented 4 years ago

Why standard (in the linux /usr/include) define

define F_OK 0 / test for existence of file /

define X_OK 0x01 / test for execute or search permission /

define W_OK 0x02 / test for write permission /

define R_OK 0x04 / test for read permission /

But https://github.com/klange/toaruos/blob/master/base/usr/include/fcntl.h#L20 define

define F_OK 1 <---- ??????

define R_OK 4

define W_OK 2

define X_OK 1

ZanyXDev commented 4 years ago

Test app `

include

include

include

include

include

include

int main(int argc, char * argv[]) { printf("\nUsage: %s [test_file]",argv[0]); printf("\nFile %s:",argv[1]);

int result;

result = access( argv[1], F_OK );
printf("\nAccess result F_OK: %d",result);

if( result != -1 ) {
    printf("\nTest for existence - SUCCESS!");
} else {
     printf("\nTest for existence - FAILLURE!");
}

result = access( argv[1], X_OK );
printf("\nAccess result X_OK: %d",result);

if(  result != -1 ) {
    printf("\nTest for execute or search permission - SUCCESS!");
} else {
     printf("\nTest for execute or search permission - FAILLURE!");
}

result = access( argv[1], W_OK );
printf("\nAccess result W_OK: %d",result);

if(  result != -1 ) {
    printf("\nTest for write permission - SUCCESS!");
} else {
     printf("\nTest for write permission - FAILLURE!");
}

result = access( argv[1], R_OK );
printf("\nAccess result R_OK: %d",result);

if(  result != -1 ) {
    printf("\nTest for read permission - SUCCESS!");
} else {
     printf("\nTest for read permission - FAILLURE!");
}
exit(0);

} ` functions int access(const char *pathname, int mode) return -2 if file not found 0 If the file exists, permissions (read, write) are not checked.

klange commented 4 years ago

I have no idea what your goal is in testing access. fopen happily returns NULL when it fails to open a file, so access isn't necessary to resolve this issue.

#define F_OK 1 <---- ??????

Probably because it's a typo. I'll fix it.

-2 if file not found

-2 is -ENOENT so it would seem the system call result is leaking.

klange commented 4 years ago

Additionally, all of your comments here are poorly formatted which makes them difficult to read.

ZanyXDev commented 4 years ago

Checks:

klange commented 4 years ago

Once again, I'm not sure what your goal is here. I have already told you that the system call underlying that libc function doesn't even check the mode argument, so of course the result will always be the same regardless of what you pass there.

ZanyXDev commented 4 years ago

I already realized that the problem with checking user rights to access a file (Read, Write, Execute) is at the level of system calls. But if you don't mind, I will continue in this thread. You wrote:

system call only checks if the file can be opened.

but fs_node_t * node = kopen((char *)file, 0); where 0 it is flags = O_RDONLY returns valid node_ptr simple if file found without check permissions. Example /etc/sudoers: -rw------- 1 root root 6 Aug 08 10:33 sudoers system call sys_access return 0 -> file can be opened, but user not permission to read file and cat /etc/sudoers returns cat: /etc/sudoers: Permisssion denied

klange commented 4 years ago

What does any of this have to do with imgviewer segfaulting without an argument? You have gone off on a tangent with unrelated things.

ZanyXDev commented 4 years ago

Using access(), can check for the existence of the file, but if the file is exists, and the user does not have permissions to access -> imgviewer terminated with segmentation fault

klange commented 4 years ago

You do not need to use access for this at all. The graphics library function should be changed or replaced and the call to fopen can do all the work for you as the underlying open system call will check permissions. access is a broken interface anyway as it’s subject to time-of-use bugs.

ZanyXDev commented 4 years ago

Ok. In my opinion, the error is here FILE * image = fopen(filename, "r"); size_t image_size= 0; fseek(image, 0, SEEK_END); <--- segmentation fault If the user does not have access rights to the file, then fopen () returns an invalid pointer to the FILE*.

ZanyXDev commented 4 years ago

Why load_sprite return void but load_sprite_jpg return int

klange commented 4 years ago

Because I actually thought about this problem when I wrote load_sprite_jpg a year ago and didn't care about it seven years ago when I wrote load_sprite.