Weird code crash

I think that is conclusive.

It seems to have been a double free caused by lack of defensive coding plus an asynch timed file write function causing the temporary creation of an empty file, or perhaps no file at all.

Yes

No.

I think that is exactly the case. I didnt think that was in fact possible

Yes, I was just wondering that before I read this post. Rename unlinks the old file does it?

I might implement that, as well. It doesn't really matter however, as in practice the structures than contain thermometer data don't get altered if no valid data is found, so the lack of a proper file, ex of causing a crash, now simply means the (unused in this program) name data gets erased. For a few seconds. It simply misses a reading and uses last times data for everything else. Mostly the temperature.

I think the consensus is that it does.

Presumably if the read process has the old file open, that will be valid until it closes it?

• The Natural Philosopher snipped-for-privacy@invalid.invalid | On 15/09/2023 15:27, Ralf Fassel wrote: | > * The Natural Philosopher snipped-for-privacy@invalid.invalid | > | > | thermometers[i].name=strdup(p); // | > | > | make a copy of the name and attach it | > | > | to our thermometer structure | > | > Memory leak if thermometers[i].name already contains something. | > | >

| > | further up the line... | >>

| > | bzero(filbuf,sizeof(filbuf)); | > | /** first thing to do is clean any allocated memory used to | > | store values. **/ | > | for(i=0;i<NUMBER_RELAYS;i++) | > | free(thermometers[i].name); | > Note that the assignment | > thermometers[i].name=strdup(p); | > is *inside* the while() loop without a free(). | > Probably you argue that there ever is only a single file to read in | > that dir anyway... Personally, I've been bitten by such assumptions, so I'd | > rather check once too often than hunting down "can't happen" bugs. | > R' | > | No. you have misunderstood how the code works.

Sorry, but I have to give that compliment back. You describe how the code is _intended_ to work. I described how the code _actually_ works.

It all depends on what files with which content are there in that directory, so if there ever is only one file per ZONE, all is peachy. If not, all bets are off.

Not 100% seriously, may I refer you to

| (It would be trivial to simply add a conditional that only strdups to | a pointer if it is NULL).

With char* malloc'd pointers, I find it much easier to simply stick to the pattern:

- initialize to 0

- free before reassignment

- assign to 0 after free when not directly reassigning instead of arguing at each place why not sticking to the pattern is not a problem.

| However they are not at this time misconfigured, so it shouldn't be | the crash problem, [...]

Agreed.

| I do think that what has happened is that a valid file name has been | found with empty data, or no file at all, and then no strdup is done - | but the free is, next time around.

Easy to verify via diagnostics, just add a stderr-output for every unexpected situation (such as the same index seen twice etc).

| As is allocating memory only if the pointers are null.

Why not simply free()/strdup()? If you assign to 0 only, you may get old contents for the new file inside the loop (can't happen, I know :-)!

R'

• The Natural Philosopher snipped-for-privacy@invalid.invalid | On 15/09/2023 16:12, vallor wrote: | > Meanwhile, if you want to avoid locking your file, you might want to | > write | > a fresh file with a unique name, then rename() it, | > which -- please correct me if I'm wrong -- should replace | > the desired file atomically.

| I think the consensus is that it does.

| Presumably if the read process has the old file open, that will be | valid until it closes it?

On Linux: yes. Once a process has a file open, it sees the 'old' contents if the file is removed from disk.

On Fri, 15 Sep 2023 16:46:43 +0100, The Natural Philosopher snipped-for-privacy@invalid.invalid wrote in <ue1u93$3a7pg$ snipped-for-privacy@dont-email.me:

Yes -- and the old file remains allocated on disk until its file descriptor is closed.

On Fri, 15 Sep 2023 18:19:13 +0200, Ralf Fassel snipped-for-privacy@gmx.de wrote in snipped-for-privacy@akutech.de:

Speaking of which: back in the days of Linux yore, you could retrieve the contents of a delete file if a process still had it open through: /proc/##/fd/*.

(Nowadays, those are symlinks.)

That's why I take steps to avoid it. Since files and dynamically- allocated memory are global resources, I declare file and memory pointers as global variables. I initialize them to NULL as an indication that the file is not open or memory is not allocated. If fopen() or malloc() fails, the pointer remains NULL and serves as an indication of failure. When I close a file or free a block of memory, I immediately set the corresponding pointer to NULL. All my programs exit through a routine I call quit_cleanup(), which closes files or frees memory for any non-NULL pointer before calling exit(). I can call this function at any time (e.g. to force abnormal termination) and everything will be appropriately freed. Similarly, I can check a pointer for non-NULL before opening a file or allocating memory (although realloc() takes care of that automatically). It's quite effective in avoiding resource leaks as well as the undefined behaviour mentioned above.

I've always deleted the original file before doing the rename, probably because under MS-DOS or Windows the rename will fail if the original file already exists.

As a side note, don't use this technique if you're porting your code to Windows, where the delete/rename paradigm is permanently and incurably unreliable. When you ask Windows to delete a file, it queues the request and doesn't actually delete the file until it feels like it. Sometimes this doesn't happen until after you've attempted the rename, which then fails because the original file is still there. Some time afterwards, Windows gets around to doing the delete, and once your cleanup routine gets rid of the work file, your data is gone for good.

This doesn't happen very often, but even a 0.01% failure rate can result in a trouble call every week or two if your program is running daily at 1000 customer sites.

I discovered this little "feature" back in the Windows 95 days, and as far as I know it's still there. The only safe workaround is to copy the contents of the work file back to the original file. For small files the time difference is neglegible, and for large files... well, too bad, so sad. Suck it up and wait. Or convince the customer to switch to Linux.

It is. One of the points where Linux evaluates to determe if it should task switch is upon exit from a syscall. If your writer process runs out its timeslice during the in-kernel portion of the work for an fopen, then the kernel will suspend it and schedule another process to run. You now have an empty, unwritten file on disk which will not be written to until the writer is next scheduled by the kernel. If the next process scheduled is the reader, and it was last suspended just before it did an fopen() on this same file, it will now fopen() an empty file.

Yes: (man 2 rename):

If newpath already exists, it will be atomically replaced, so that there is no point at which another process attempting to access newpath will find it missing. However, there will probably be a window in which both oldpath and newpath refer to the file being renamed.

Yes, your temperature monitoring was unaffected. But if the race was sometimes triggering the pointer double-free that your loop previously had, then the lack of atomicity was at least one trigger for the intermittent crash.

So seems like two routes to fix:

For something that you'll potentially want to 'just run' for months/years on end without daily care and feeding, doing both is the better defense.