@MSGID: <ygazg1nh0sm.fsf@akutech.de> 49bca71f
@REPLY: 1@dont-email.me> f56398d4
@REPLYADDR Ralf Fassel <ralfixx@gmx.de>
@REPLYTO 2:5075/128 Ralf Fassel
@CHRS: CP866 2
@RFC: 1 0
@RFC-Message-ID: <ygazg1nh0sm.fsf@akutech.de>
@RFC-References: 1@dont-email.me>
<ygamsxoixhx.fsf@akutech.de>4@dont-email.me> <ygail8biyxm.fsf@akutech.de>1@dont-email.me>
<ygaedizitb9.fsf@akutech.de>2@dont-email.me> <yga4jjvik9o.fsf@akutech.de>1@dont-email.me>
@TZUTC: 0200
@PID: Gnus/5.13 (Gnus v5.13) Emacs/27.2
(gnu/linux)
@TID: FIDOGATE-5.12-ge4e8b94
* 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 | > | 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`
--- Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
 * Origin: usenet.network (2:5075/128)
SEEN-BY: 5001/100 5005/49 5015/255 5019/40 5020/715
848 1042 4441 12000
SEEN-BY: 5030/49 1081 5058/104 5075/128
@PATH: 5075/128 5020/1042 4441