I just stumbled over this myself, testing a build of XQuartz on mac OS 10.13 within the MacPorts system.

FYI I have pushed a possible patch for the issue (for use in MacPorts) to 

https://github.com/macports/macports-ports/pull/2499

https://github.com/macports/macports-ports/blob/a92e97fdcfcdaf8b18991d888e4fb6a851892a18/x11/xorg-server-devel/files/fix-calloc-free-mis-match-bug.patch

Basically, the elements in the fbconfigs 'array' are individually allocated, instead of via a single bulk calloc call. In this way the loop that subsequently frees them is then valid.

FWIW, a patch for MacPorts could just as be skipping the free loop - if my analysis is correct that this is done only on exit. The patch you're proposing looks like a true fix that could be upstreamed.

I do have to ask however, is there a point in freeing this probably small amount of memory that would in any case be freed by the runtime, beyond "good practice"? Are there still systems on which XOrg might run where allocated memory gets "lost" when not freed explicitly?

It is bad practise to leak memory intentionally, particularly when not doing so is easy to fix.

Ah, but the memory isn't being leaked in the true sense of the term; it'll be released a wee bit later anyway.

5 years ago I would have agreed, but a couple of years more experience with C++ and really complex software have made much more relaxed. Certain things indeed have to be closed/freed properly, but if the system (or runtime) can do the cleanup there's no shame in relying on that. It means the cleanup code is already there, so why burden your own code with a duplicate that could lead to subtle race conditions or even "stupid" bugs like this one?

Actually C++ specifically is one place where you want to ensure that all destructors are properly called, and this doesn't happen if you let the OS free up the resources instead of destroying things yourself. And I don't like the idea of doing it even with other languages (unsurprising, since I'm the author of the patch ;-)).

I think the _clean_ solution to this would be to make the visuals array destructor an indirect function call, where most implementations would use the default one that just goes through the array freeing elements one by one, while the macOS one does  the single free using the initial pointer. Or would this be overengineering the issue?

(In reply to Giuseppe Bilotta from comment #5)
> Actually C++ specifically is one place where you want to ensure that all
> destructors are properly called, and this doesn't happen if you let the OS
> free up the resources instead of destroying things yourself.

I'm not so sure about that, not with modern compilers and runtimes. I've been seeing too many race conditions lately where destructors were called (and tried to access already freed resources) *after* exiting from main() (i.e. in the global destruction phase). Specifically, with Qt and KDE software, and Qt has stopped unloading plugins explicitly in order to avoid race conditions because it's become near impossible to guarantee their absence.
C++ *is* particular in the sense that destructors can do more than just free memory, but the implications of that can go both ways if you see what I mean.

> one, while the macOS one does  the single free using the initial pointer. Or
> would this be overengineering the issue?

Yes - as I said in my OP. It's less complex than the solution I thought about, but it would still be a lot simpler to make a formal requirement that all list elements are allocated individually. The Mac approach of adding elements from an array is a bit of a hack anyway. An approach that can be clever if you own both the list and the array but a bit of a recipe for disaster if you don't.
Besides, your proposal opens the door to implementations NOT freeing the elements, and that's apparently something you wouldn't like to be responsible for ;)

Rene - A leak is always a leak and never acceptable. It does not matter if it happens just before the application quits. Its still a leak. For one thing any future change in the code based could move that leak from the exit to anywhere else in the code base, so relying on the system cleaning up after you is short-sighted and poor programming. Its *always* possible to engineer a good memory manage model into your application (even easier with model C++ standards, with the likes of std::unique_ptr etc.). SO personnally, I will never accept an argument along the lines of 'its OK to leak cos the system will clean up for me.'

Giuseppe - Your suggestion seems reasonable but beyond what I aimed to fix. The patch I made fixes things for OSX which was my concern. I posted it just for info, How to do this 'properly' is for you guys to decide ;)

I think of memory leaks as something causing unnecessary memory loss, either until the application exits or else until you reboot the system. Skipping an explicit free when you know someone else will do it for you moments later is not leaking in that view. You can consider it as such, but then you really should also stop using local/automatic variables too (because who knows how large and long-lived their context might become) ;)

But, I don't really disagree with you - reread my remark about being certain that the memory is indeed going to be freed (= only running the loop at exit, in this case). Oh, and "never acceptable" .. indeed, unless it's the least unacceptable solution ;)

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/212.