[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialog
From: |
H.G. Muller |
Subject: |
Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue |
Date: |
Sat, 21 Mar 2015 11:48:58 +0100 |
User-agent: |
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
Thomas Adam schreef op 3/21/2015 om 12:06 AM:
I see---it seems a little... hmm, odd, that the presense of the dialogue
is the thing to check. That there is no data in glc seems the more
technically correct thing to check for.
I think the two are always correlated, as XBoard always created the
dialog as soon
as it gets a non-empty game list. I guess at the time it felt natural to
test for this,
because I assumed that the actual segfault was due to accessing a
non-existent widget.
So I added a test for its existence, I could have been wrong about that,
though.
It bothers me, because it's crufty, and often that looks bad, especially
in terms of code structure, etc.
Well, I am not saying that this should not be done. I just explained I
am not very motivated
to do it. I am basically only interested in adding new functionality,
and then usually because
it is functionality I need myself as an XBoard/WinBoard user. So it is
all the better if there
are other people that would be interested in cleaning up the code.
(Which was already in
a pretty sorry state by the time I got involved.)
I'm starting this already [1], there's a few topic branches where I've
been able to make a start. It's actually rather nice doing this because
it's already flagged up a lot of interesting points which are on my
radar, for instance these things (in no particular order):
* if (f) free(f) -- free(NULL) is guaranteed to be a no-op;
* free() versus FREE() versus free(f); f = NULL;
* [cm]alloc() -> snprintf() -- just use asprintf();
* strcpy / etc.
The FREE and ASSIGN macros are things I added myself relatively
recently, because I got
a bit tired from writing "if(f) free(f); f=dup(x);" all the time. So in
new code I use those.
But all the old code still writes it explicitly; I did not see any point
in replacing working code,
as thie macros were intended to save me work, not produce it.
It would be good to also have a macro DEBUG for the construct
"if(appData.debugMode") fprintf(debugFP, ", which is also something I
have to type
annoyingly often.
I wasn't even aware something like asprintf existed. I don't know if
there is a particular
reason avoiding it. (Like compatibility with old libraries that do not
have it. Amazingly
enough even snprintf seems to give problems in MicroSoft Visual Studio.)
In code I add
I usually print to declare static or automatic arrays, rather than to
allocated memory.
At one point we tried to replace all strcpy by safeStrCpy. But being
dogmatic about that
often leads to very silly code, like safeStrCpy(*command, 512, "go\n");
where there is
no doubt at all that the 3 printed characters will fit in the target,
and you have to assume
the size of the target anyway, because it is only available as (char*).
Things like
safeStrCpy(*command, strlen("go\n"), "go\n") are even worse. So I still
use strcpy
sometimes for cases where there is no doubt at all that there cannot be
any buffer overflow.
That was a question I had: I do not have access to a windows PC. How
do you ensure changes made doesn't break Winboard? Indeed, whilst I
might *just* about be able to cross-compile it under Linux, I certainly
couldn't check the run-time results. So any thing I do would only be
with respect to XBoard itself.
Well, this is a problem. Anything change in the back-end that would
require a compensating
change in the XBoard front-end are likely to break WinBoard, if the
WinBoard front-end
would not get the compensating change too. A weaker problem is
functional equivalence;
it is undesirable if features added to XBoard are not operational in
WinBoard for lack of
front-end functionality, even though they compile OK.
- [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Thomas Adam, 2015/03/20
- [XBoard-devel] [PATCH 2/2] GameListPrepare: Change return value to void, Thomas Adam, 2015/03/20
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, H.G. Muller, 2015/03/20
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Thomas Adam, 2015/03/20
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue,
H.G. Muller <=
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Thomas Adam, 2015/03/21
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, H.G. Muller, 2015/03/21
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Joshua Pettus, 2015/03/21
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Thomas Adam, 2015/03/21
- Re: [XBoard-devel] [PATCH 1/2] Fix segfault with 'Game List Tags' dialogue, Tim Mann, 2015/03/22