littlekernel / lk

LK embedded kernel
MIT License
3.11k stars 613 forks source link

Question about the fat32_mount function and the management of dynamically allocated memory #262

Open tec-quark opened 4 years ago

tec-quark commented 4 years ago

Hi, sorry for my English, I am using an online translator, I have a doubt about memory management in the fat.c file precisely in the fat32_mount function. I would like to know what happens with the memory allocated to the fat_fs_t * fat pointer in the event of an error. How is memory freed?


    fat_fs_t * fat = malloc (sizeof (fat_fs_t));
    fat-> lba_start = 1024;
    fat-> dev = dev;

    fat-> bytes_per_sector = fat_read16 (bs, 0xb);
    if ((fat-> bytes_per_sector! = 0x200) && (fat-> bytes_per_sector! = 0x400) && (fat-> bytes_per_sector! = 0x800)) {
        printf ("unsupported sector size (% x) \ n", fat-> bytes_per_sector);
        result = ERR_NOT_VALID;
        goto end;

Thank you very much Santiago

mu578 commented 4 years ago

never it's a leak, find usage within code and see what happens if fat32_mount fails; how it is handled; if fat32_unmount is called anyways a simple patch by moving cookie assignment after the end label will suffice; else a free(fat_device) is necessary before bailing out to end label i.e go to.

tec-quark commented 4 years ago

Thank you for your answer and suggestion. I'd like to try and emulate the error to see what happens, but I preferred to ask this question here to get an opinion from people who have much more experience than me. I will do it. Also I would like to add some code to check the correct memory allocation by making a simple if (fat == null), for security. Thanks again for your suggestion. A greeting

mu578 commented 4 years ago

you must set cookie to null else you'll hit dandling condition; there is no such thing as implicit state/assignment in C.

tec-quark commented 4 years ago

Yes, sorry, I misunderstood myself, I was also referring to this section of the code

    fat_fs_t * fat = malloc (sizeof (fat_fs_t));
// here
    fat-> lba_start = 1024;
    fat-> dev = dev;
mu578 commented 4 years ago

yes I know; no need to be sorry; you did understand well; I went further more; I'll always do; if you are able to grasp the first step I'll always imply next; there is also another malloc unsafe call to deal with and all the logic derived from it; this code is student grade (I suppose, as I never experienced such step in my life, so the true reasons behind such state/result always remain fuzzy to me).

travisg commented 4 years ago

Indeed it looks like that code is in some need of cleanup for error cases. Any patches are welcome to help clean it up a bit.

tec-quark commented 4 years ago

Hi, yes, I was encouraged by moe123 and I have already forked the project (which I find fantastic) and I am making some changes on the code. Then I will try to understand how to make the code visible, this is the first time that I work on an external project :) Thanks.

if my english is not good ... then it's the fault of the online translator :)

mu578 commented 4 years ago

@tec-quark your translator is doing fine and well; better than I do sometimes. However, if you have a task within a source base, first don't let yourself distracted go for it.

Few informal advices: when changing a code. There are always at least two steps:

Meanwhile, in this particular situation there is a third one: fat is part of a larger family. Does all members of this family are homogeneous and comply to the same rulesets at usage.

[*] you may perfectly emulate it on your desktop as long as it is a unix-like, devices can be seen as plain file.