t-w / ADFlib

A free, portable and open implementation of the Amiga filesystem
GNU General Public License v2.0
0 stars 1 forks source link

Uninitialized or garbage access #7

Closed DidierMalenfant closed 7 months ago

DidierMalenfant commented 7 months ago

These should definitely be reviewed by someone with a better knowledge of the codebase, just in case... 😀

t-w commented 7 months ago

3635a35 - such order is not to read root block before things above are done (if disk is full, there is nothing to do). But, indeed, vol probably should be passed there as const struct adfVolume * const - but I remember that I ran on some annoying thing where compiler was complaining about incompatible types. The problem was, if I remember well, that const in C is a part of the type, not an annotation to function prototype saying how the function is dealing with its argument. So giving struct s * const as argument to function having const struct s * const is causing trouble (warnings), if I remember well, while it shouldn't...

So I guess here it will be better to ignore what Xcode is saying...

I am trying to make as many things const as possible since this prevents bugs, but sometimes I hit a wall that forces me to at least postpone it in certain cases, as it gives more trouble that profit. This is one of them.

t-w commented 7 months ago

51ce799 - this makes no sense to me... The whole block is read by adfReadFSHDblock. Why initialize anything before???

Where is this warning coming from? Some Xcode static analyzer?

From my experience, most static analyzers gives usually a lot false information. So, really, reading literally their output and making changes just to silence them is not a good practice... I suggest rather really carefully look at the code first before making any changes and ignore/silence warnings if they are not valid.

t-w commented 7 months ago

920b898 - ok. (though separating the check for each would let reporting which one fails, with numbers)

DidierMalenfant commented 7 months ago

Hey there... I'm going to close these. I imagine you probably don't mean to but, you're coming across as making a lot of assumptions about where I'm coming from and my level of expertise in software development in general.

This is your project. You should absolutely do what you think is best. I'll keep posting fixes and changes to my working branch and you can cherry pick anything you want off it if you want.

I wish you the best with this.

t-w commented 7 months ago

Hi, please have a look at my (longer) comment in the other PR, as it applies here, too.

Best wishes.

t-w commented 7 months ago

And 97e2d2f is also OK.

t-w commented 6 months ago

After the second look, I cherry-picked these.

Some of these things fix details where some more attention and testing may be needed, there are still pieces I haven't touched yet... Some of such things you changed in these PRs.

Anyway - thank you for these.

Recently I had to do many changes across the whole lib (like to make naming more consistent), there should be less of that since now, so it should be easier with PRs.