kdenetwork/ksirc

[Andrew Stanley-Jones]

This was CC'ed to core devel a few days ago, so I thought I might as well 
follow up.  While my first patch was wrong, but  this brings up a very old 
topic, and maybe it could refresh some memories.  

The change to the code (strncpy->strlcpy) was simple and even though it 
"should not" do anything bad, broke the entire program making it useless.* 
Since the change was made by someone who did not attempt to understand the 
code, and then never tested the problem wasn't found till much later.  I 
fixed head since I ran accross it.

Yesterday while on irc on #kde-devel a little message poped up, "anyone tried 
kSirc in 3.1 branch recently?"  Client was completly broken.  Again a simple 
fix, and we got it good and early so no harm done.

But it really bugs me when changes are done and not tested.  I understand that 
when changing lots of code all over the place it would be very time 
consuming, but especially in stable branches how else do you know it was 
good?  If you don't understand the code/or are unwilling to test it maybe 
email the maintainer?

</rant>

I don't mean to bash on anyone, and their efforts are excelent, but I just 
want to use this oportunity to offer a friendly reminder.

Cheers), 

-Andrew

*: the strings being copied were not supposed to be null terminated, and were 
being moved around using string+length.  The addition of the null at the end 
caused all strings being sent by the client to be corrupt.

On Monday 16 December 2002 09:22 am, you wrote:
> On Monday 16 of December 2002 17:57, Andrew Standley-Jones wrote:
> > CVS commit by asj:
> >
> > Make it work with 3.1 libs
>
>  No. In order to make it work with 3.1 libs, you have to update your admin/
> dir. Before another one of you goes to change strlcpy back to strncpy,
> because something doesn't compile, first check that you're up to date, and
> then please complain to me.
>
>  And since people ask, the full story is here :
> http://www.courtesan.com/todd/papers/strlcpy.html
>
>  In short, strlcpy and strlcat are what strncpy and strncat probably were
> meant to be. You simply pass the total buffer size as the last argument,
> and they'll take care of the rest. Both strncpy and strncat are quite
> poorly designed and are easy to use incorrectly.
>
>  In order to use strlcpy/strlcat, the code has to #include <config.h>,
> because most platforms don't have it, and the code also has to link to
> either libkdecore or libkdefakes.
>
>  The rules for usage are simple:
> 1) avoid plain char* texts and use Q(C)String
> or
> 2) always check you have enough space in buffers
> or
> 3) use strlcpy and strlcat
>
>  That's about it.
>
>  I will now check that KDE_3_0_BRANCH and newer have the configure check
> and the code in fakes, and the next person doing similar change will have
> to discuss it with my bazooka >;).
>
> >   M +6 -1      iocontroller.cpp   1.47
> >
> >
> > --- kdenetwork/ksirc/iocontroller.cpp  #1.46:1.47
> > @@ -75,4 +75,5 @@
> >
> >  #include <kdebug.h>
> > +#include <kdeversion.h>
> >  #include <kprocess.h>
> >  #include <kstandarddirs.h>
> > @@ -249,5 +250,9 @@ void KSircIOController::stdin_write(QCSt
> >      }
> >      send_buf = new char[len];
> > +#if KDE_VERSION > 310
> >      strlcpy(send_buf, buffer.data(), len);
> > +#else
> > +    strncpy(send_buf, buffer.data(), len);
> > +#endif
> >      if(proc->writeStdin(send_buf, len) == FALSE){
> >        kdDebug() << "Failed to write but CTS HIGH! Setting low!: " << s
> > << endl;

-- 
---
Andrew Stanley-Jones         | "It's kind of fun to do the impossible."
EE, LongEz N87KJ             |                           -- Walt Disney