Memory leaks and allocation fixes

[DidierMalenfant]

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

[t-w]

First, some notes to keep in mind in general (it was somewhat expressed in README, but not that literally):

The library code, in a large part, is rather old. Some parts are still not tested / reviewed / used (by me, at least). As you can imagine, changing those parts has to be done carefully, cannot be done solely on the base of a compiler or a code analyzer / linter report. The real meaning of each part has to be analyzed, what, in many cases, is not trivial as some states tend to change in many places. It slowly changes as, to make this reasonably, I focus on changes in some module/subsystem and I try to make it cleaner doing improvements.

Therefore, the changes that I apply in those little known and not tested parts are either minor refactorings with no semantic changes (reformatting, changes done across the whole lib, like names) or really small, simple and "obvious" things that does not need tests.

Any more meaningful changes, for instance in adf_salv, must be done in depth, with understanding what really happens there, followed by tests, which, if possible, should become also part of the testing suite. Otherwise, making changes can cause more harm than good - the changed code may lose its true meaning (which was and is the value of the library - that, even if not without some flaws, it does many things well) and it will be necessary to figure out whether the problem comes from the original code or the correction done because a code analyzer suggested so...

So, please keep this in mind while making changes. Be rather sure what you are submitting, as I probably won't be able to merge changes that fixes one static code analyzer warning but make the code work completely differently and there are no tests that can validate it.

This concerns especially 2 modules: adf_salv, adf_cache, partially also adf_dir (possibly others if the particular piece/functionality has no tests, esp. unit tests which are usually more extensive).

I will have a closer look at these as, after a brief look, I see they do point out things to verify - but the change required there is not necessarily the one I see here...

[DidierMalenfant]

I've done my part as carefully as I could, I'm trying to get the unit tests running in Xcode to cover that too.

Those look, again to me, like flat out bugs. But it's always best to have this looked over by someone more senior with the codebase.

I've included them in my work branch, so I will report back if I come across any issues with my project. I'll let you decide in the meantime if you want to merge them or not.

[t-w]

Unfortunately, tests (including unit tests) do not cover everything. So it is not possible to make changes anywhere and consider done if they pass. The most tested part is data access - reading and writing files in all possible ways, and things related to it. Other, less used or additional things may have very limited or no tests at all. adf_salv (data recovery) and adf_cache (directory cache) are such cases.

Do (or will) you use functionality from adf_salv? Myself, I have not (yet...). Those things are very specific, not for general purpose data access. They are for data recovery and repairing the filesystem. These kind of operations are always tricky and without very thorough tests, more complex changes just to get rid of a warning will likely make things worse.

So I suggest first to focus on functionality that is really used, therefore - tested, rather than "cleaning-up" the code that we cannot be sure if and how it works. If you will not be using that part, please be very cautious with its modifications.

[t-w]

803a3b6:

RETCODE adfCheckParent ( struct AdfVolume * vol,
                         SECTNUM            pSect )
{
[...]
1.   struct GenBlock* block = (struct GenBlock*)malloc(sizeof(struct GenBlock));

2.   block->name = NULL;

3.   if ( block == NULL) {
        (*adfEnv.wFct)("adfCheckParent : malloc failed");
        return RC_ERROR;
    }

really, this order?

There is a leak on block->name, indeed. But simple free(block->name) in adfCheckParent will do, and ensuring NULL or proper pointer in adfReadGenBlock.

This is exactly what I was writing about above. Each such change has to be done very carefully, just "masking" the warnings does no good...

And I can be wrong, concerning the solution, too, I haven't tested that ;-)

Can you check these once more and let me know if I should go on?

(abd82b2 is OK).

[DidierMalenfant]

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]

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.

Hi. I am not making any assumptions. I do apologize if it felt that way. Your expertise can be possibly way better than mine, I cannot know that... But, for instance, the thing I pointed out above (a mistake anyone can make, a human thing) is something I would need to correct myself, if I merge it like this. So I wanted to encourage you to improve it.

I just want things to be fixed, not covered, as this will make more trouble. I did couple of time this kind of "compiler warning" corrections without checking things deep enough and then had a lot of work figuring out a bug related to it. Just because I haven't checked the thing enough, and did not have enough tests.

If you are dealing with the code perfectly crafted, you can just simply fix on every warning reported on every change as it is less likely to have deeper problem (though it is not excluded). But with older and, in some places, prototype-style code, changes needs to be done carefully, for the reasons I mentioned before (internal dependencies are sometimes too tricky).

Your contributions are valuable, so definitely I didn't mean to discourage you from it. But also the PRs have to be done properly so they do not add more work addressing the real issues.

If you are uncertain how to fix some problem you encounter (not because of any assumption about the level of expertise - but, as you mentioned starting this PR, the code itself, which is in some places unnecessarily complex and error prone, I myself do not know it fully, either...), it is probably better to submit an issue with a suggestion how to fix it (that's what I do myself in many cases), than make it a part of a larger PR. Other way - submit a PR. but only with that uncertain fix so that it can be treated/discussed/improved separately, while other things can be merged quickly (as it happened with your other PRs). It may seem to go on slower like this, but it might not be so, but it will surely be working towards the right way of solving things (IMHO).

The things you caught definitely need to be checked in their context and fixed anyway. And there are many other things that can be improved, so... It is up to you if you want to contribute. You are welcomed if you do.

[t-w]

Except for patches for adfGetFileBlocks, which I had to refactor heavily, I cherry-picked your changes (some with minor corrections, like what I mentioned above).

Thanks!