Thread: [Sc2-devel] strcpy to strncpy

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

same as last patch but for strcpy

--Abaddon

On 6 Dec 2002, Abaddon wrote:
> same as last patch but for strcpy
strncpy does not null-terminate if the string is too short. This one needs
work.

Serge

right you are (i remembered for strncat, in any case, this behaviour is
better than overwriting everything, and it will more than likely result
in a crash from reading memory out of bounds rather than writing memory
out of bounds...)

in any event, ill change them to sizeof -1...

--Abaddon

On Fri, 2002-12-06 at 05:01, Serge van den Boom wrote:
> On 6 Dec 2002, Abaddon wrote:
> > same as last patch but for strcpy
> strncpy does not null-terminate if the string is too short. This one need=
s
> work.
>=20
> Serge
>=20
>=20
>=20
>=20
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Sc2-devel mailing list
> Sc2...@li...
> https://lists.sourceforge.net/lists/listinfo/sc2-devel
>=20

On 6 Dec 2002, Abaddon wrote:
> right you are (i remembered for strncat, in any case, this behaviour is
> better than overwriting everything, and it will more than likely result
> in a crash from reading memory out of bounds rather than writing memory
> out of bounds...)
>
> in any event, ill change them to sizeof -1...
That's not enough. You need to add the '\0' char manually. But as the string
doesn't make sense when cut it off, it's better to generate an error when
this happens (terminate the program probably). As strncpy doesn't provide
a way to check this [1], it might be better to check the length in advance.
(Note that it's safe to use strcpy in that case.)
Actually, continuing with the string cut off isn't correct behaviour for
the strncat and snprintf patches either. (though for snprintf you can check
the return value).
What might be better for the strcpy is to use an own version of strdup().
Not strdup() itself as it might fail, requiring more checks everywhere (it's
not portable anyhow).

Serge

1) Not totally accurate, you could set the last char to '\0' before the call
   and check if it's still there afterwards.

ok, heres the thing...these fixes are to effect stability, in a proper
running environment they should never overrun anyways, and up to this
point everyone has been just hoping that was the case...these patches
are just to effect stability...more to the point they are to make the
propagation of bugs harder...

the original code doesn't even check to see if its going to fit, with
these patches it will always fit, it might crash a second later if its
not null terminated but it didn't trash your stack or your heap...the
problem is that this often doesn't crash the system but causes data
corruption...

tell you what, you guys decide how you want this fixed and ill fix it,
until then these patches are far far better than just not checking...

but when you do finally decide how this is best fixed, please please
please please, make sure everyone from then on out follows the
conventions because these patches, while i am more than happy to do
them, are very time consuming, boring, and tedious to make and test...

--Abaddon

On 6 Dec 2002, Abaddon wrote:
> ok, heres the thing...these fixes are to effect stability, in a proper
> running environment they should never overrun anyways, and up to this
> point everyone has been just hoping that was the case...these patches
> are just to effect stability...more to the point they are to make the
> propagation of bugs harder...
>
> the original code doesn't even check to see if its going to fit, with
> these patches it will always fit, it might crash a second later if its
> not null terminated but it didn't trash your stack or your heap...the
> problem is that this often doesn't crash the system but causes data
> corruption...
Yeah, I understand this must be annoying to you. And I agree that the sort
of incorrect behaviour caused by cut off strings is usually easier to debug
than that caused by a corrupted heap. However, wasn't the idea of these
patches to get these things fixed once and for all?

If you don't want to fix this, I'll apply your previous patches, as they
ARE better than what we have right now, but I'd be much happier if you'd
take another look at it. It's your choice.

> tell you what, you guys decide how you want this fixed and ill fix it,
I'd like to see too long strings be detected and handled.
How you do it, I don't mind much. The stuff I mentioned in my previous mail
were only suggestions. (just keep in mind it needs to run on various
platforms).

> but when you do finally decide how this is best fixed, please please
> please please, make sure everyone from then on out follows the
> conventions because these patches,
We've got the 'Contributing' file now for precisely this reason.

Serge

Btw, I'm at the moment catching up with some other work I should have done
before, when I was working on UQM. Don't expect me to have much time to do
many commits for a few days.

look, i agree with everything you've said, and i will apply fixes
however you want, in the mean time apply the first round of patches,
then tell me what you want done, because im not going to go off, do it
one way which i think is perfectly reasonable, and then have everyone
say no when im done...tell me the way that will make you happy and ill
do it that way, once and for all, i have no problem doing that, but i do
need to know up front what you would find amicable...

--Abaddon

On Fri, 2002-12-06 at 14:04, Serge van den Boom wrote:
> On 6 Dec 2002, Abaddon wrote:
> > ok, heres the thing...these fixes are to effect stability, in a proper
> > running environment they should never overrun anyways, and up to this
> > point everyone has been just hoping that was the case...these patches
> > are just to effect stability...more to the point they are to make the
> > propagation of bugs harder...
> >
> > the original code doesn't even check to see if its going to fit, with
> > these patches it will always fit, it might crash a second later if its
> > not null terminated but it didn't trash your stack or your heap...the
> > problem is that this often doesn't crash the system but causes data
> > corruption...
> Yeah, I understand this must be annoying to you. And I agree that the sor=
t
> of incorrect behaviour caused by cut off strings is usually easier to deb=
ug
> than that caused by a corrupted heap. However, wasn't the idea of these
> patches to get these things fixed once and for all?
>=20
> If you don't want to fix this, I'll apply your previous patches, as they
> ARE better than what we have right now, but I'd be much happier if you'd
> take another look at it. It's your choice.
>=20
> > tell you what, you guys decide how you want this fixed and ill fix it,
> I'd like to see too long strings be detected and handled.
> How you do it, I don't mind much. The stuff I mentioned in my previous ma=
il
> were only suggestions. (just keep in mind it needs to run on various
> platforms).
>=20
> > but when you do finally decide how this is best fixed, please please
> > please please, make sure everyone from then on out follows the
> > conventions because these patches,
> We've got the 'Contributing' file now for precisely this reason.
>=20
> Serge
>=20
>=20
> Btw, I'm at the moment catching up with some other work I should have don=
e
> before, when I was working on UQM. Don't expect me to have much time to d=
o
> many commits for a few days.
>=20
>=20
>=20
>=20
>=20
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Sc2-devel mailing list
> Sc2...@li...
> https://lists.sourceforge.net/lists/listinfo/sc2-devel
>=20

done...

--Abaddon

On Fri, 2002-12-06 at 05:01, Serge van den Boom wrote:
> On 6 Dec 2002, Abaddon wrote:
> > same as last patch but for strcpy
> strncpy does not null-terminate if the string is too short. This one needs
> work.
> 
> Serge
> 
> 
> 
> 
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Sc2-devel mailing list
> Sc2...@li...
> https://lists.sourceforge.net/lists/listinfo/sc2-devel
> 

done...

--Abaddon

On Fri, 2002-12-06 at 05:01, Serge van den Boom wrote:
> On 6 Dec 2002, Abaddon wrote:
> > same as last patch but for strcpy
> strncpy does not null-terminate if the string is too short. This one needs
> work.
> 
> Serge
> 
> 
> 
> 
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Sc2-devel mailing list
> Sc2...@li...
> https://lists.sourceforge.net/lists/listinfo/sc2-devel
> 