* The Natural Philosopher <tnp@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'

On 15/09/2023 14:23, Theo wrote:
> You could get a SIGABRT if you were trying to free something that was
> already freed. Are you sure those are interlocked such that for each i you
> call strdup() exactly once, and subsequently free() exactly once? If there
> was some code path that was breaking out of the loop or similar you might
> get such behaviour.

Well, I am not sure if that was it or not, but I deleted manually a
thermometer file and the thing crashed instantly. That is consistent
with the name having been set once, and then repeatedly free()ed. I then
installed the code with the free()ed pointers set to NULL, and it
*didn't* crash instantly.

I had assumed that freeing a pointer that already had been freed would
either result in a NO-OP because the pointer no longer existed in the
heap memory allocation tables, or it would instantly crash , but it
seems that the action is 'undefined'.

Not sure that's done the trick, because I don't quite see how a file
could ever cease to exist.

To not exist in the first place is one thing, but once written, nothing
should delete them.

Unless fopen("w") does that for a fraction of a microsecond

Or fopen("w") creates an *empty* file, in which case it is *just*
possible that an empty file is read, no strdup was done and the pointer
was double freed...next time around.

Academic now anyway. Pointers all set to null after freeing. Defined
behaviour. frees on NULL ignored.

I'll let it run and run and see.

--
The biggest threat to humanity comes from socialism, which has utterly
diverted our attention away from what really matters to our existential
survival, to indulging in navel gazing and faux moral investigations
into what the world ought to be, whilst we fail utterly to deal with
what it actually is.

On 9/15/23 08:23, Theo wrote:
> In comp.sys.raspberry-pi The Natural Philosopher <tnp@invalid.invalid> wrote:
>> On 15/09/2023 12:12, Ralf Fassel wrote:
>>> | {
>>> | *q++=0;
>>> | 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);
>
> You could get a SIGABRT if you were trying to free something that was
> already freed. Are you sure those are interlocked such that for each i you
> call strdup() exactly once, and subsequently free() exactly once? If there
> was some code path that was breaking out of the loop or similar you might
> get such behaviour.
>
> Theo

I thought double free was a SIGSEGV?
--
--
user <candycane> is generated from /dev/urandom

On 15/09/2023 15:27, Ralf Fassel wrote:
> * The Natural Philosopher <tnp@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.

There are up to 4 (NUMBER_RELAYS) thermometer files in that dir, and all
of them are read in the loop. What there shouldn't be is more than one
file with a ZONE number the same. So no pointer gets more than one STRDUP

If there were, it might be possible to strdup the same pointer twice.
And the daemon would get a memory leak and crash.

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

That is a possibility that could be caused by mis-configuration of the
thermometers themselves.

However they are not at this time misconfigured, so it shouldn't be the
crash problem, although it is an issue I will consider because fat
fingers *could* cause it.

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.

That should never happen of course, as the fopen/fwrite sequence should
certainly not delete the filename, but it is entirely possible that a
the fopen *truncates* its data. At which point we cant strdup anything,
so the next free gets a woopsie

Setting the pointers to NULL after free() is nice defensive coding

As is allocating memory only if the pointers are null.

So both are in there now.

--
“Progress is precisely that which rules and regulations did not foresee,”

– Ludwig von Mises

In comp.os.linux.misc The Natural Philosopher <tnp@invalid.invalid> wrote:
> I had assumed that freeing a pointer that already had been freed would
> either result in a NO-OP because the pointer no longer existed in the
> heap memory allocation tables, or it would instantly crash , but it
> seems that the action is 'undefined'.

Yes, C explicitly labels "double free" as "undefined":

<http://port70.net/~nsz/c/c99/n1256.html#J.2>

Look under J.2 Undefined behavior (easiest is to search for "free"):

J.2 Undefined behavior

1 The behavior is undefined in the following circumstances:

...

The pointer argument to the free or realloc function does not match
a pointer earlier returned by calloc, malloc, or realloc, or the
space has been deallocated by a call to free or realloc (7.20.3.2,
7.20.3.4).

And th 7.20.3.2 link in the page jumps to this:

The free function causes the space pointed to by ptr to be
deallocated, that is, made available for further allocation. If
ptr is a null pointer, no action occurs. Otherwise, if the
argument does not match a pointer earlier returned by the calloc,
malloc, or realloc function, or if the space has been deallocated
by a call to free or realloc, the behavior is undefined.

So if by chance you are double-freeing sometimes, then you are tickling
the undefined behaviour devil, and all bets are off as to what might
eventually occur.

In comp.os.linux.misc candycanearter07 <no@thanks.net> wrote:
>
> I thought double free was a SIGSEGV?

Check my other reply to TNP for the details, but it is "undefined" in
C.

On 15/09/2023 15:40, candycanearter07 wrote:
> On 9/15/23 08:23, Theo wrote:
>> In comp.sys.raspberry-pi The Natural Philosopher <tnp@invalid.invalid>
>> wrote:
>>> On 15/09/2023 12:12, Ralf Fassel wrote:
>>>> |                             {
>>>> |                             *q++=0;
>>>> |                             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);
>>
>> You could get a SIGABRT if you were trying to free something that was
>> already freed.  Are you sure those are interlocked such that for each
>> i you
>> call strdup() exactly once, and subsequently free() exactly once?  If
>> there
>> was some code path that was breaking out of the loop or similar you might
>> get such behaviour.
>>
>> Theo
>
> I thought double free was a SIGSEGV?

In fact it seems fairly undefined

It looks like it is somewhat implementation dependent. SIGSEGV means you
accessed unallocated memory, but that is not the same as freeing
allocated memory, twice.

There seem to be instances of it reported. Google is a friend here.

I *suspect* that if that is the problem, its a signal from deep within libc.
Whereas SIGSEGV probably emanates from a memory management unit somewhere

--
"Strange as it seems, no amount of learning can cure stupidity, and
higher education positively fortifies it."

- Stephen Vizinczey

candycanearter07 <no@thanks.net> writes:
> On 9/15/23 08:23, Theo wrote:
>> You could get a SIGABRT if you were trying to free something that was
>> already freed. Are you sure those are interlocked such that for each
>> i you call strdup() exactly once, and subsequently free() exactly
>> once? If there was some code path that was breaking out of the loop
>> or similar you might get such behaviour.
>
> I thought double free was a SIGSEGV?

If Glibc detects it you’ll get a diagnostic and SIGABRT.

If it doesn’t detect it then anything could happen - SIGSEGV is just one
possibility.

--
https://www.greenend.org.uk/rjk/

On Fri, 15 Sep 2023 14:56:23 +0100, The Natural Philosopher
<tnp@invalid.invalid> wrote in <ue1nq7$39033$1@dont-email.me>:

> On 15/09/2023 14:23, Theo wrote:
>> In comp.sys.raspberry-pi The Natural Philosopher <tnp@invalid.invalid>
>> wrote:
>>> On 15/09/2023 12:12, Ralf Fassel wrote:
>>>> | {
>>>> | *q++=0;
>>>> | 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);
>>
>> You could get a SIGABRT if you were trying to free something that was
>> already freed. Are you sure those are interlocked such that for each i
>> you call strdup() exactly once, and subsequently free() exactly once?
>> If there was some code path that was breaking out of the loop or
>> similar you might get such behaviour.
>>
> Hmm. I free the pointers even for relay zones that don't have
> thermometers, whose pointers are 0. That isn't an issue.
>
> But that might be a remotely possible issue. I dont zero the pointers
> after freeing them as far as I can tell. The silly thing is that this
> program doesn't use the name anyway.
>
> Its used elsewhere Well I don't think its an issue, but I can zero the
> pointers anyway after free()ing
>
>> Theo

Hi, read the thread with interest.

If you're getting SIGABRT, that's almost always the software
calling abort(3). If you aren't, maybe there's a library calling it?

$ man 7 signal
[...]
Signal Standard Action Comment
SIGABRT P1990 Core Abort signal from abort(3)
[but it also lists]
SIGIOT - Core IOT trap. A synonym for SIGABRT
_ _ _ _ _ _ _

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.

--
-v

On Fri, 15 Sep 2023 12:19:45 +0100, Pancho wrote:

> Personally, I want to run with full debug, stack trace, logging,
> exception handling, and bounds checking turned on all the time, even in
> production. Which is why I generally use a modern language like C# or
> Java.
>
Same here. Many years back I wrote the type of debugging and programming
support library I personally find most useful: it can report the content
of all common variable types as well as dumping byte arrays as both hex
and ASCII as well as parsing the command line and allow the amount of
debug info the be controlled by a command line argument.

They are structured as small libraries that designed to be lightweight
enough to be left in a program when its in general use.

The library was originally written in C, but I soon wrote a Java version
as well, though this hasn't been separately published yet.

If this sounds useful, both versions can be found on www.libelle-
systems.com in the "Free Stuff" section.

--

Martin | martin at
Gregorie | gregorie dot org

In comp.os.linux.misc The Natural Philosopher <tnp@invalid.invalid> wrote:
> On 15/09/2023 15:27, Ralf Fassel wrote:
>> 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.
>>
> 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.
>
> That should never happen of course, as the fopen/fwrite sequence
> should certainly not delete the filename, but it is entirely possible
> that a the fopen *truncates* its data. At which point we cant strdup
> anything, so the next free gets a woopsie

Are the "files" being written to by an independent process separate
from this reading process?

If yes, are you doing any form of locking/synchronization to prevent
the reading process from trying to read from a file that a writing
process has open/truncated, but not yet written any data into?

If no, then you may be also hitting a race condition where the stars
align just right, the writer has just performed its fopen/truncate
(leaving the file empty) and the kernel decides to context switch away
to the reader at that point, before the writer can write and close the
file. The reader will then see an empty file.

The classic "lock free" solution to this one is for the writer to
create and write to a temporary file, and after closing the temp file
to rename() it to the name of the real file. Rename is documented to
be atomic, so the reader would never see a half open, or partially
complete, file in this case.

On 15/09/2023 16:09, Richard Kettlewell wrote:
> candycanearter07 <no@thanks.net> writes:
>> On 9/15/23 08:23, Theo wrote:
>>> You could get a SIGABRT if you were trying to free something that was
>>> already freed. Are you sure those are interlocked such that for each
>>> i you call strdup() exactly once, and subsequently free() exactly
>>> once? If there was some code path that was breaking out of the loop
>>> or similar you might get such behaviour.
>>
>> I thought double free was a SIGSEGV?
>
> If Glibc detects it you’ll get a diagnostic and SIGABRT.

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.

>
> If it doesn’t detect it then anything could happen - SIGSEGV is just one
> possibility.
>

--
I would rather have questions that cannot be answered...
....than to have answers that cannot be questioned

Richard Feynman

On 15/09/2023 16:26, Rich wrote:
> In comp.os.linux.misc The Natural Philosopher <tnp@invalid.invalid> wrote:
>> On 15/09/2023 15:27, Ralf Fassel wrote:
>>> 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.
>>>
>> 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.
>>
>> That should never happen of course, as the fopen/fwrite sequence
>> should certainly not delete the filename, but it is entirely possible
>> that a the fopen *truncates* its data. At which point we cant strdup
>> anything, so the next free gets a woopsie
>
> Are the "files" being written to by an independent process separate
> from this reading process?
>
Yes

> If yes, are you doing any form of locking/synchronization to prevent
> the reading process from trying to read from a file that a writing
> process has open/truncated, but not yet written any data into?
>
No.

> If no, then you may be also hitting a race condition where the stars
> align just right, the writer has just performed its fopen/truncate
> (leaving the file empty) and the kernel decides to context switch away
> to the reader at that point, before the writer can write and close the
> file. The reader will then see an empty file.
>
I think that is exactly the case. I didnt think that was in fact possible

> The classic "lock free" solution to this one is for the writer to
> create and write to a temporary file, and after closing the temp file
> to rename() it to the name of the real file. Rename is documented to
> be atomic, so the reader would never see a half open, or partially
> complete, file in this case.

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.

--
Truth welcomes investigation because truth knows investigation will lead
to converts. It is deception that uses all the other techniques.

On 15/09/2023 16:12, vallor wrote:
> On Fri, 15 Sep 2023 14:56:23 +0100, The Natural Philosopher
> <tnp@invalid.invalid> wrote in <ue1nq7$39033$1@dont-email.me>:
>
>> On 15/09/2023 14:23, Theo wrote:
>>> In comp.sys.raspberry-pi The Natural Philosopher <tnp@invalid.invalid>
>>> wrote:
>>>> On 15/09/2023 12:12, Ralf Fassel wrote:
>>>>> | {
>>>>> | *q++=0;
>>>>> | 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);
>>>
>>> You could get a SIGABRT if you were trying to free something that was
>>> already freed. Are you sure those are interlocked such that for each i
>>> you call strdup() exactly once, and subsequently free() exactly once?
>>> If there was some code path that was breaking out of the loop or
>>> similar you might get such behaviour.
>>>
>> Hmm. I free the pointers even for relay zones that don't have
>> thermometers, whose pointers are 0. That isn't an issue.
>>
>> But that might be a remotely possible issue. I dont zero the pointers
>> after freeing them as far as I can tell. The silly thing is that this
>> program doesn't use the name anyway.
>>
>> Its used elsewhere Well I don't think its an issue, but I can zero the
>> pointers anyway after free()ing
>>
>>> Theo
>
> Hi, read the thread with interest.
>
> If you're getting SIGABRT, that's almost always the software
> calling abort(3). If you aren't, maybe there's a library calling it?
>
> $ man 7 signal
> [...]
> Signal Standard Action Comment
> SIGABRT P1990 Core Abort signal from abort(3)
> [but it also lists]
> SIGIOT - Core IOT trap. A synonym for SIGABRT
> _ _ _ _ _ _ _
>
> 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?

--
"I guess a rattlesnake ain't risponsible fer bein' a rattlesnake, but ah
puts mah heel on um jess the same if'n I catches him around mah chillun".

* The Natural Philosopher <tnp@invalid.invalid>
| On 15/09/2023 15:27, Ralf Fassel wrote:
| > * The Natural Philosopher <tnp@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
https://core.tcl-lang.org/tips/doc/trunk/tip/131.md
;-)

| (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 <tnp@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.

https://stackoverflow.com/questions/2028874/what-happens-to-an-open-file-handle-on-linux-if-the-pointed-file-gets-moved-or-d

R'

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

> On 15/09/2023 16:12, vallor wrote:
>> On Fri, 15 Sep 2023 14:56:23 +0100, The Natural Philosopher
>> <tnp@invalid.invalid> wrote in <ue1nq7$39033$1@dont-email.me>:
>>
>>> On 15/09/2023 14:23, Theo wrote:
>>>> In comp.sys.raspberry-pi The Natural Philosopher
>>>> <tnp@invalid.invalid> wrote:
>>>>> On 15/09/2023 12:12, Ralf Fassel wrote:
>>>>>> | {
>>>>>> | *q++=0;
>>>>>> | 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);
>>>>
>>>> You could get a SIGABRT if you were trying to free something that was
>>>> already freed. Are you sure those are interlocked such that for each
>>>> i you call strdup() exactly once, and subsequently free() exactly
>>>> once? If there was some code path that was breaking out of the loop
>>>> or similar you might get such behaviour.
>>>>
>>> Hmm. I free the pointers even for relay zones that don't have
>>> thermometers, whose pointers are 0. That isn't an issue.
>>>
>>> But that might be a remotely possible issue. I dont zero the pointers
>>> after freeing them as far as I can tell. The silly thing is that this
>>> program doesn't use the name anyway.
>>>
>>> Its used elsewhere Well I don't think its an issue, but I can zero the
>>> pointers anyway after free()ing
>>>
>>>> Theo
>>
>> Hi, read the thread with interest.
>>
>> If you're getting SIGABRT, that's almost always the software calling
>> abort(3). If you aren't, maybe there's a library calling it?
>>
>> $ man 7 signal [...]
>> Signal Standard Action Comment SIGABRT P1990
>> Core Abort signal from abort(3)
>> [but it also lists]
>> SIGIOT - Core IOT trap. A synonym for SIGABRT
>> _ _ _ _ _ _ _
>>
>> 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?

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

--
-v

On Fri, 15 Sep 2023 18:19:13 +0200, Ralf Fassel <ralfixx@gmx.de> wrote in
<ygav8cbh0ji.fsf@akutech.de>:

> * The Natural Philosopher <tnp@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.
>
> https://stackoverflow.com/questions/2028874/what-happens-to-an-open-
file-handle-on-linux-if-the-pointed-file-gets-moved-or-d
>
> R'

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.)

--
-v

On 2023-09-15, Rich <rich@example.invalid> wrote:

> In comp.os.linux.misc The Natural Philosopher <tnp@invalid.invalid> wrote:
>
>> I had assumed that freeing a pointer that already had been freed would
>> either result in a NO-OP because the pointer no longer existed in the
>> heap memory allocation tables, or it would instantly crash , but it
>> seems that the action is 'undefined'.
>
> Yes, C explicitly labels "double free" as "undefined":
>
> <http://port70.net/~nsz/c/c99/n1256.html#J.2>
>
> Look under J.2 Undefined behavior (easiest is to search for "free"):
>
> J.2 Undefined behavior
>
> 1 The behavior is undefined in the following circumstances:
>
> ...
>
> The pointer argument to the free or realloc function does not match
> a pointer earlier returned by calloc, malloc, or realloc, or the
> space has been deallocated by a call to free or realloc (7.20.3.2,
> 7.20.3.4).
>
> And th 7.20.3.2 link in the page jumps to this:
>
> The free function causes the space pointed to by ptr to be
> deallocated, that is, made available for further allocation. If
> ptr is a null pointer, no action occurs. Otherwise, if the
> argument does not match a pointer earlier returned by the calloc,
> malloc, or realloc function, or if the space has been deallocated
> by a call to free or realloc, the behavior is undefined.
>
> So if by chance you are double-freeing sometimes, then you are tickling
> the undefined behaviour devil, and all bets are off as to what might
> eventually occur.

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.

--
/~\ Charlie Gibbs | They offer a huge range of
\ / <cgibbs@kltpzyxm.invalid> | world-class vulnerabilities
X I'm really at ac.dekanfrus | that only Microsoft can provide.
/ \ if you read it the right way. | -- druck <news@druck.org.uk>

On 2023-09-15, Rich <rich@example.invalid> wrote:

> The classic "lock free" solution to this one is for the writer to
> create and write to a temporary file, and after closing the temp file
> to rename() it to the name of the real file. Rename is documented to
> be atomic, so the reader would never see a half open, or partially
> complete, file in this case.

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.

--
/~\ Charlie Gibbs | They offer a huge range of
\ / <cgibbs@kltpzyxm.invalid> | world-class vulnerabilities
X I'm really at ac.dekanfrus | that only Microsoft can provide.
/ \ if you read it the right way. | -- druck <news@druck.org.uk>

In comp.os.linux.misc The Natural Philosopher <tnp@invalid.invalid> wrote:
> On 15/09/2023 16:26, Rich wrote:
>> Are the "files" being written to by an independent process separate
>> from this reading process?
>>
> Yes
>
>> If yes, are you doing any form of locking/synchronization to prevent
>> the reading process from trying to read from a file that a writing
>> process has open/truncated, but not yet written any data into?
>>
> No.
>
>> If no, then you may be also hitting a race condition where the stars
>> align just right, the writer has just performed its fopen/truncate
>> (leaving the file empty) and the kernel decides to context switch
>> away to the reader at that point, before the writer can write and
>> close the file. The reader will then see an empty file.
>>
> I think that is exactly the case. I didnt think that was in fact
> possible

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.

>> The classic "lock free" solution to this one is for the writer to
>> create and write to a temporary file, and after closing the temp file
>> to rename() it to the name of the real file. Rename is documented to
>> be atomic, so the reader would never see a half open, or partially
>> complete, file in this case.
>
> Yes, I was just wondering that before I read this post. Rename unlinks
> the old file does it?

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.

> 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.

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:

1) remove the conditions that can cause a double-free to occur in the
code (seems like you've already done this from other posts)

2) use rename() to move newly written files into place for the reader,
so the reader never opens an empty file (exclusive of the writer
crashing before it wrote anything to the file).

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.