| Message ID | cbfd7cc5cc8d4c5fbbb8f14a30745f93@de.bosch.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | AW: AW: [swupdate] [PATCH] cpio_utils: fix infinite read when interrupting an update | expand |
Hi Mark, On 21/09/2018 12:50, Jonas Mark (BT-FIR/ENG1) wrote: > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 40ae474..ef5e30c 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -135,7 +135,10 @@ static int input_step(void *state, void *buffer, size_t size) > size = s->nbytes; > } > int ret = fill_buffer(s->fdin, buffer, size, s->offs, &s->checksum, s->dgst); > - if (ret < 0) { > + if (ret <= 0) { > + if (ret == 0) { > + s->eof = true; > + } > return ret; > } > s->nbytes -= size; > > Hi Stefano, > >>>>>> When using the IPC API to do an update from a stream, a new update >>>>>> cannot be started if the previous one is interrupted. >>>>>> >>>>>> The root cause is: when the client closes the socket, the read on the >>>>>> server side will return 0 and it is treated as a successful read. Thus >>>>>> it goes into an infinite read loop. > >>>> We think that gunzip_step() has a fatal problem when ret == 0. If >>>> upstream_step() returns 0, then s->strm.avail_in will be 0, and then >>>> inflate() has nothing to do. Thus the output buffer will not be used >>>> and outlen is 0. This will cause that the loop continues because >>>> nothing was done. But the next call of upstream_step() will return 0 >>>> again because the client does not deliver data any more. This is the >>>> live lock we are experiencing. >> >> Ok, I have not tested, but this is plausible. Anyway, it looks to me >> strange how you think to fix this. The decompression step, as the other >> ones, check for an EOF condition: >> >> 241 if (s->eof) { >> 242 break; >> 243 } >> >> This looks to me the natural way for handling the case. The thing is >> that the input step does not set the eof flag, causing the endless loop >> you detect - instead of hiding the status in the return code as in >> patch, we should clearly state that a eof occurs and this should be managed. >> >> What about just to set s->eof in input step ? > > Sounds good to me. Is this what you mean? > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 40ae474..ef5e30c 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -135,7 +135,10 @@ static int input_step(void *state, void *buffer, size_t size) > size = s->nbytes; > } > int ret = fill_buffer(s->fdin, buffer, size, s->offs, &s->checksum, s->dgst); > - if (ret < 0) { > + if (ret <= 0) { > + if (ret == 0) { > + s->eof = true; > + } > return ret; > } > s->nbytes -= size; > Right, this is what I meant. >>> How do we continue with this issue? Have you been able to reproduce the >> problem using the additional information (compressed image) we gave? >> >> I have not (yet) tested, but you explanation makes things much more clearer. > > I won't be able to further testing today. We will test the proposal and > I plan getting back to you on Tuesday. ok, thanks ! Best regards, Stefano
diff --git a/core/cpio_utils.c b/core/cpio_utils.c index 40ae474..ef5e30c 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -135,7 +135,10 @@ static int input_step(void *state, void *buffer, size_t size) size = s->nbytes; } int ret = fill_buffer(s->fdin, buffer, size, s->offs, &s->checksum, s->dgst); - if (ret < 0) { + if (ret <= 0) { + if (ret == 0) { + s->eof = true; + } return ret; } s->nbytes -= size; Hi Stefano, > >>>> When using the IPC API to do an update from a stream, a new update > >>>> cannot be started if the previous one is interrupted. > >>>> > >>>> The root cause is: when the client closes the socket, the read on the > >>>> server side will return 0 and it is treated as a successful read. Thus > >>>> it goes into an infinite read loop. > >> We think that gunzip_step() has a fatal problem when ret == 0. If > >> upstream_step() returns 0, then s->strm.avail_in will be 0, and then > >> inflate() has nothing to do. Thus the output buffer will not be used > >> and outlen is 0. This will cause that the loop continues because > >> nothing was done. But the next call of upstream_step() will return 0 > >> again because the client does not deliver data any more. This is the > >> live lock we are experiencing. > > Ok, I have not tested, but this is plausible. Anyway, it looks to me > strange how you think to fix this. The decompression step, as the other > ones, check for an EOF condition: > > 241 if (s->eof) { > 242 break; > 243 } > > This looks to me the natural way for handling the case. The thing is > that the input step does not set the eof flag, causing the endless loop > you detect - instead of hiding the status in the return code as in > patch, we should clearly state that a eof occurs and this should be managed. > > What about just to set s->eof in input step ? Sounds good to me. Is this what you mean? diff --git a/core/cpio_utils.c b/core/cpio_utils.c index 40ae474..ef5e30c 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -135,7 +135,10 @@ static int input_step(void *state, void *buffer, size_t size) size = s->nbytes; } int ret = fill_buffer(s->fdin, buffer, size, s->offs, &s->checksum, s->dgst); - if (ret < 0) { + if (ret <= 0) { + if (ret == 0) { + s->eof = true; + } return ret; } s->nbytes -= size;