[Libreoffice-commits] core.git: desktop/source

[Matteo Casalin]

On Wed, 20 Feb 2013 15:32:10 +0100
Matteo Casalin <matteo.casalin at gmx.com> wrote:

> Hi all,
>     I may be completely wrong and I really don't know this code and what it is aimet to, but I'm worried that
> 
>     if (nBytes > 0)
>     {
>         pReceiveBuffer[nBytes-1] = 0;
>     }
> 
> overwrites a received character (that could be '\0' or not).
> 
> If I understand correctly, the target is to have the sequence of characters being '\0' terminated whether that '\0' whas in the stream or not, and then I would say that the whole routine should be something like:
> 
>     char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};

As an additional thought, this could then be :

      char pReceiveBuffer[sc_nCSASeqLength + 1];

>     int nResult = 0;
>     int nBytes = 0;
>     // read byte per byte
>     while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes, sc_nCSASeqLength-nBytes))>0)
>     {
>         nBytes += nResult;
>         if (pReceiveBuffer[nBytes-1]=='\0')
>         {
>             break;
>         }
>     }
>     // unconditionally end stream with '\0'
>     pReceiveBuffer[nBytes] = '\0';
> 
> In case a '\0' is already in the sequence and the additional '\0' is undesired, nBytes can be adjusted in the "if (pReceiveBuffer[nBytes-1]=='\0')" so to overwrite the existing one.
> Also, the "read byte per byte" comment looks wrong to me.
> 
> If I misunderstood the point then apologies for the noise.
> Cheers
> 
> Matteo
> 
> 
> 
> On Wed, 20 Feb 2013 05:54:50 -0800 (PST)
> julien2412 <serval2412 at yahoo.fr> wrote:
> 
> > Lubos Lunak wrote
> > > On Tuesday 19 of February 2013, Julien Nabet wrote:
> > >> diff --git a/desktop/source/app/officeipcthread.cxx
> > >> b/desktop/source/app/officeipcthread.cxx index 8db7946..445ccb4 100644
> > >> --- a/desktop/source/app/officeipcthread.cxx
> > >> +++ b/desktop/source/app/officeipcthread.cxx
> > >> @@ -497,23 +497,17 @@ OfficeIPCThread::Status
> > >> OfficeIPCThread::EnableOfficeIPCThread() else if( pThread->maPipe.create(
> > >> aPipeIdent.getStr(), osl_Pipe_OPEN, rSecurity )) // Creation not
> > >> successfull, now we try to connect {
> > >>              osl::StreamPipe aStreamPipe(pThread->maPipe.getHandle());
> > >> -            char pReceiveBuffer[sc_nCSASeqLength + 1];
> > >> +            char pReceiveBuffer[sc_nCSASeqLength + 1] = {0};
> > >>              int nResult = 0;
> > >>              int nBytes = 0;
> > >>              int nBufSz = sc_nCSASeqLength + 1;
> > >>              // read byte per byte
> > >> -            pReceiveBuffer[0] = 0;
> > >>              while ((nResult=aStreamPipe.recv( pReceiveBuffer+nBytes,
> > >> nBufSz-nBytes))>0) { nBytes += nResult;
> > >>                  if (pReceiveBuffer[nBytes-1]=='\0') {
> > >>                      break;
> > >>                  }
> > >>              }
> > >> -            /* make sure the buffer is \0 terminated */
> > >> -            if (nBytes > 0)
> > >> -            {
> > >> -                pReceiveBuffer[nBytes-1] = 0;
> > >> -            }
> > >  Did you really mean to remove this part?
> > 
> > Hi Lubos,
> > 
> > Yes I meant it, why? Is it wrong?
> > if "pReceiveBuffer" is initialized with 0 for the (sc_nCSASeqLength + 1)
> > elements thanks to = {0} initialization, what obvious thing did I miss? Why
> > "pReceiveBuffer[nBytes-1] = 0;" would need to stay?
> > 
> > Of course, if my commit is wrong, I'll revert it after my day time job
> > 
> > Julien
> > 
> > 
> > 
> > --
> > View this message in context: http://nabble.documentfoundation.org/Re-Libreoffice-commits-core-git-desktop-source-tp4038892p4038893.html
> > Sent from the Dev mailing list archive at Nabble.com.
> > _______________________________________________
> > LibreOffice mailing list
> > LibreOffice at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/libreoffice
> _______________________________________________
> LibreOffice mailing list
> LibreOffice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice