| Message ID | 20200316160702.478964-2-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gcc 9.2 strncpy(3) warnings fixes | expand |
Stefan Hajnoczi <stefanha@redhat.com> wrote: > gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled > reports the following error: > > CC migration/global_state.o > In file included from /usr/include/string.h:495, > from /home/stefanha/qemu/include/qemu/osdep.h:101, > from migration/global_state.c:13: > In function ‘strncpy’, > inlined from ‘global_state_store_running’ at migration/global_state.c:47:5: > /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation] > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate > strings. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> This is the film where: - one put a size 100 - none of the current elements has a size of 20 - for extra security one put one assert to make sure tha there are space. And compiler still find a reason for complaining O:-)
On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: > gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled > reports the following error: > > CC migration/global_state.o > In file included from /usr/include/string.h:495, > from /home/stefanha/qemu/include/qemu/osdep.h:101, > from migration/global_state.c:13: > In function ‘strncpy’, > inlined from ‘global_state_store_running’ at migration/global_state.c:47:5: > /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation] > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate > strings. There was a long discussion 1 year ago with it, and Eric suggested to use strpadcpy after the assert() and I sent this patch: https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html Not sure what's best. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > migration/global_state.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/migration/global_state.c b/migration/global_state.c > index 25311479a4..cbe07f21a8 100644 > --- a/migration/global_state.c > +++ b/migration/global_state.c > @@ -44,8 +44,8 @@ void global_state_store_running(void) > { > const char *state = RunState_str(RUN_STATE_RUNNING); > assert(strlen(state) < sizeof(global_state.runstate)); > - strncpy((char *)global_state.runstate, > - state, sizeof(global_state.runstate)); > + pstrcpy((char *)global_state.runstate, > + sizeof(global_state.runstate), state); > } > > bool global_state_received(void) >
On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: >> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled >> reports the following error: >> >> CC migration/global_state.o >> In file included from /usr/include/string.h:495, >> from /home/stefanha/qemu/include/qemu/osdep.h:101, >> from migration/global_state.c:13: >> In function ‘strncpy’, >> inlined from ‘global_state_store_running’ at >> migration/global_state.c:47:5: >> /usr/include/bits/string_fortified.h:106:10: error: >> ‘__builtin_strncpy’ specified bound 100 equals destination size >> [-Werror=stringop-truncation] >> 106 | return __builtin___strncpy_chk (__dest, __src, __len, >> __bos (__dest)); >> | >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate >> strings. > > There was a long discussion 1 year ago with it, and Eric suggested to > use strpadcpy after the assert() and I sent this patch: > https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html > Not sure what's best. strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we guarantee the contents of all bytes in the buffer. pstrcpy() does not (but pstrcpy() can be followed up with a memset() to emulate the remaining effects of strncpy() - at which point you have reimplemented strpadcpy). > >> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> migration/global_state.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/migration/global_state.c b/migration/global_state.c >> index 25311479a4..cbe07f21a8 100644 >> --- a/migration/global_state.c >> +++ b/migration/global_state.c >> @@ -44,8 +44,8 @@ void global_state_store_running(void) >> { >> const char *state = RunState_str(RUN_STATE_RUNNING); >> assert(strlen(state) < sizeof(global_state.runstate)); >> - strncpy((char *)global_state.runstate, >> - state, sizeof(global_state.runstate)); >> + pstrcpy((char *)global_state.runstate, >> + sizeof(global_state.runstate), state); Can we guarantee that the padding bytes have been previously set to 0, or do we need to go the extra mile with a memset() or strpadcpy() to guarantee that we have set the entire buffer?
On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote: > On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: > > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: > > > gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled > > > reports the following error: > > > > > > CC migration/global_state.o > > > In file included from /usr/include/string.h:495, > > > from /home/stefanha/qemu/include/qemu/osdep.h:101, > > > from migration/global_state.c:13: > > > In function ‘strncpy’, > > > inlined from ‘global_state_store_running’ at > > > migration/global_state.c:47:5: > > > /usr/include/bits/string_fortified.h:106:10: error: > > > ‘__builtin_strncpy’ specified bound 100 equals destination size > > > [-Werror=stringop-truncation] > > > 106 | return __builtin___strncpy_chk (__dest, __src, __len, > > > __bos (__dest)); > > > | > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate > > > strings. > > > > There was a long discussion 1 year ago with it, and Eric suggested to > > use strpadcpy after the assert() and I sent this patch: > > https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html > > Not sure what's best. > > strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we > guarantee the contents of all bytes in the buffer. pstrcpy() does not (but > pstrcpy() can be followed up with a memset() to emulate the remaining > effects of strncpy() - at which point you have reimplemented strpadcpy). > > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > migration/global_state.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/migration/global_state.c b/migration/global_state.c > > > index 25311479a4..cbe07f21a8 100644 > > > --- a/migration/global_state.c > > > +++ b/migration/global_state.c > > > @@ -44,8 +44,8 @@ void global_state_store_running(void) > > > { > > > const char *state = RunState_str(RUN_STATE_RUNNING); > > > assert(strlen(state) < sizeof(global_state.runstate)); > > > - strncpy((char *)global_state.runstate, > > > - state, sizeof(global_state.runstate)); > > > + pstrcpy((char *)global_state.runstate, > > > + sizeof(global_state.runstate), state); > > Can we guarantee that the padding bytes have been previously set to 0, or do > we need to go the extra mile with a memset() or strpadcpy() to guarantee > that we have set the entire buffer? I don't understand GlobalState: 1. You ask if runstate[] must be padded with NULs but neither global_state_store() nor register_global_state() do that. Is it really necessary to pad runstate[]? If yes, is it safe for global_state_store() and register_global_state() to not pad runstate[]? If we decide the pad runstate[] to prevent information leaks to the migration destination then I think it should be done in the pre-save function so that it's guaranteed to happen no matter which of the 3 functions that write runstate[] has been called. 2. There is a GlobalState::size field that is only written and then migrated but never read from what I can tell. :? Juan: Please clarify the above. Thanks!
On 3/17/20 10:52 AM, Stefan Hajnoczi wrote: > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote: >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: >>> On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: >>>> gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled >>>> reports the following error: >>>> >>>> CC migration/global_state.o >>>> In file included from /usr/include/string.h:495, >>>> from /home/stefanha/qemu/include/qemu/osdep.h:101, >>>> from migration/global_state.c:13: >>>> In function ‘strncpy’, >>>> inlined from ‘global_state_store_running’ at >>>> migration/global_state.c:47:5: >>>> /usr/include/bits/string_fortified.h:106:10: error: >>>> ‘__builtin_strncpy’ specified bound 100 equals destination size >>>> [-Werror=stringop-truncation] >>>> 106 | return __builtin___strncpy_chk (__dest, __src, __len, >>>> __bos (__dest)); >>>> | >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> >>>> Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate >>>> strings. >>> >>> There was a long discussion 1 year ago with it, and Eric suggested to >>> use strpadcpy after the assert() and I sent this patch: >>> https://www.mail-archive.com/qemu-block@nongnu.org/msg44925.html >>> Not sure what's best. >> >> strncpy() pads the tail, guaranteeing that for our fixed-size buffer, we >> guarantee the contents of all bytes in the buffer. pstrcpy() does not (but >> pstrcpy() can be followed up with a memset() to emulate the remaining >> effects of strncpy() - at which point you have reimplemented strpadcpy). >> >>> >>>> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> --- >>>> migration/global_state.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/migration/global_state.c b/migration/global_state.c >>>> index 25311479a4..cbe07f21a8 100644 >>>> --- a/migration/global_state.c >>>> +++ b/migration/global_state.c >>>> @@ -44,8 +44,8 @@ void global_state_store_running(void) >>>> { >>>> const char *state = RunState_str(RUN_STATE_RUNNING); >>>> assert(strlen(state) < sizeof(global_state.runstate)); >>>> - strncpy((char *)global_state.runstate, >>>> - state, sizeof(global_state.runstate)); >>>> + pstrcpy((char *)global_state.runstate, >>>> + sizeof(global_state.runstate), state); >> >> Can we guarantee that the padding bytes have been previously set to 0, or do >> we need to go the extra mile with a memset() or strpadcpy() to guarantee >> that we have set the entire buffer? > > I don't understand GlobalState: > > 1. You ask if runstate[] must be padded with NULs but neither > global_state_store() nor register_global_state() do that. Is it > really necessary to pad runstate[]? > > If yes, is it safe for global_state_store() and > register_global_state() to not pad runstate[]? > > If we decide the pad runstate[] to prevent information leaks to the > migration destination then I think it should be done in the pre-save > function so that it's guaranteed to happen no matter which of the 3 > functions that write runstate[] has been called. > > 2. There is a GlobalState::size field that is only written and then > migrated but never read from what I can tell. :? Isn't it in process_incoming_migration_bh()? /* If global state section was not received or we are in running state, we need to obey autostart. Any other state is set with runstate_set. */ ... runstate_set(global_state_get_runstate()); > > Juan: Please clarify the above. Thanks! >
Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote: >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: >> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: >> >> > >> > > >> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> > > --- >> > > migration/global_state.c | 4 ++-- >> > > 1 file changed, 2 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/migration/global_state.c b/migration/global_state.c >> > > index 25311479a4..cbe07f21a8 100644 >> > > --- a/migration/global_state.c >> > > +++ b/migration/global_state.c >> > > @@ -44,8 +44,8 @@ void global_state_store_running(void) >> > > { >> > > const char *state = RunState_str(RUN_STATE_RUNNING); >> > > assert(strlen(state) < sizeof(global_state.runstate)); >> > > - strncpy((char *)global_state.runstate, >> > > - state, sizeof(global_state.runstate)); >> > > + pstrcpy((char *)global_state.runstate, >> > > + sizeof(global_state.runstate), state); >> >> Can we guarantee that the padding bytes have been previously set to 0, or do >> we need to go the extra mile with a memset() or strpadcpy() to guarantee >> that we have set the entire buffer? > > I don't understand GlobalState: Welcome to the club O:-) And I thought that with the reviewed-by I had finished here O:-) > 1. You ask if runstate[] must be padded with NULs but neither > global_state_store() nor register_global_state() do that. Is it > really necessary to pad runstate[]? > > If yes, is it safe for global_state_store() and > register_global_state() to not pad runstate[]? it is an error. All should be padded. > If we decide the pad runstate[] to prevent information leaks to the > migration destination then I think it should be done in the pre-save > function so that it's guaranteed to happen no matter which of the 3 > functions that write runstate[] has been called. Ok. Taking a look at this. > 2. There is a GlobalState::size field that is only written and then > migrated but never read from what I can tell. :? Grrr. It should be used, but it is not :-( What we have here: - A static buffer uint8_t runstate[100]; That is partially filled. size: is the size of that buffer that is filled. But, as we are using VMSTATE_BUFFER(runstate, GlobalState), We are always sending/receiving the whole buffer. THat is why we have trouble with padding. What should we being doing? Sending just the size, the filled bytes, and making sure that there is enough space on destination. But we aren't donig that. And at this point, I think that I am only going to fix the 1st part (always zero pad everything sent). For fixing the other bit, I need to do an incompatible change. > Juan: Please clarify the above. Thanks! Thanks a lot. Later, Juan. PD: Why is it done this way? Because at the moment, the problem was that qcow2 (as a system, not as a device) didn't have a place where to plug pending requests. So I created this section that always exist, and anything that has not a device associated can hang a subsection here. Once that I created it, nobody used it. And now, just seing what you are telling, I didn't even used the right approach.
On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote: > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote: > >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: > >> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: > >> > >> > > >> > > > >> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > >> > > --- > >> > > migration/global_state.c | 4 ++-- > >> > > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > > > >> > > diff --git a/migration/global_state.c b/migration/global_state.c > >> > > index 25311479a4..cbe07f21a8 100644 > >> > > --- a/migration/global_state.c > >> > > +++ b/migration/global_state.c > >> > > @@ -44,8 +44,8 @@ void global_state_store_running(void) > >> > > { > >> > > const char *state = RunState_str(RUN_STATE_RUNNING); > >> > > assert(strlen(state) < sizeof(global_state.runstate)); > >> > > - strncpy((char *)global_state.runstate, > >> > > - state, sizeof(global_state.runstate)); > >> > > + pstrcpy((char *)global_state.runstate, > >> > > + sizeof(global_state.runstate), state); > >> > >> Can we guarantee that the padding bytes have been previously set to 0, or do > >> we need to go the extra mile with a memset() or strpadcpy() to guarantee > >> that we have set the entire buffer? > > > > I don't understand GlobalState: > > Welcome to the club O:-) > > And I thought that with the reviewed-by I had finished here O:-) > > > 1. You ask if runstate[] must be padded with NULs but neither > > global_state_store() nor register_global_state() do that. Is it > > really necessary to pad runstate[]? > > > > If yes, is it safe for global_state_store() and > > register_global_state() to not pad runstate[]? > > it is an error. All should be padded. > > > If we decide the pad runstate[] to prevent information leaks to the > > migration destination then I think it should be done in the pre-save > > function so that it's guaranteed to happen no matter which of the 3 > > functions that write runstate[] has been called. > > Ok. > Taking a look at this. > > > 2. There is a GlobalState::size field that is only written and then > > migrated but never read from what I can tell. :? > > Grrr. It should be used, but it is not :-( > > What we have here: > - A static buffer > > uint8_t runstate[100]; > > That is partially filled. > size: is the size of that buffer that is filled. > > But, as we are using > > VMSTATE_BUFFER(runstate, GlobalState), > > We are always sending/receiving the whole buffer. THat is why we have > trouble with padding. What should we being doing? > > Sending just the size, the filled bytes, and making sure that there is > enough space on destination. > > But we aren't donig that. And at this point, I think that I am only > going to fix the 1st part (always zero pad everything sent). > > For fixing the other bit, I need to do an incompatible change. > > > Juan: Please clarify the above. Thanks! > > Thanks a lot. > > Later, Juan. > > PD: Why is it done this way? > Because at the moment, the problem was that qcow2 (as a system, not > as a device) didn't have a place where to plug pending requests. So > I created this section that always exist, and anything that has not > a device associated can hang a subsection here. Once that I created > it, nobody used it. > And now, just seing what you are telling, I didn't even used the > right approach. Great, thanks for looking into this. Could you base your patches on top of this series? Then you can send them all together in a single pull request. That way we can be sure that padding will be added even after switching to pstrcpy() in my patch. Thanks, Stefan
Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Tue, Mar 17, 2020 at 11:35:14AM +0100, Juan Quintela wrote: >> Stefan Hajnoczi <stefanha@redhat.com> wrote: >> > On Mon, Mar 16, 2020 at 01:15:35PM -0500, Eric Blake wrote: >> >> On 3/16/20 1:09 PM, Philippe Mathieu-Daudé wrote: >> >> > On 3/16/20 5:07 PM, Stefan Hajnoczi wrote: >> >> >> >> > >> >> > > >> >> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> >> > > --- >> >> > > migration/global_state.c | 4 ++-- >> >> > > 1 file changed, 2 insertions(+), 2 deletions(-) >> >> > > >> >> > > diff --git a/migration/global_state.c b/migration/global_state.c >> >> > > index 25311479a4..cbe07f21a8 100644 >> >> > > --- a/migration/global_state.c >> >> > > +++ b/migration/global_state.c >> >> > > @@ -44,8 +44,8 @@ void global_state_store_running(void) >> >> > > { >> >> > > const char *state = RunState_str(RUN_STATE_RUNNING); >> >> > > assert(strlen(state) < sizeof(global_state.runstate)); >> >> > > - strncpy((char *)global_state.runstate, >> >> > > - state, sizeof(global_state.runstate)); >> >> > > + pstrcpy((char *)global_state.runstate, >> >> > > + sizeof(global_state.runstate), state); >> >> >> >> Can we guarantee that the padding bytes have been previously set to 0, or do >> >> we need to go the extra mile with a memset() or strpadcpy() to guarantee >> >> that we have set the entire buffer? >> > >> > I don't understand GlobalState: >> >> Welcome to the club O:-) >> >> And I thought that with the reviewed-by I had finished here O:-) >> >> > 1. You ask if runstate[] must be padded with NULs but neither >> > global_state_store() nor register_global_state() do that. Is it >> > really necessary to pad runstate[]? >> > >> > If yes, is it safe for global_state_store() and >> > register_global_state() to not pad runstate[]? >> >> it is an error. All should be padded. >> >> > If we decide the pad runstate[] to prevent information leaks to the >> > migration destination then I think it should be done in the pre-save >> > function so that it's guaranteed to happen no matter which of the 3 >> > functions that write runstate[] has been called. >> >> Ok. >> Taking a look at this. >> >> > 2. There is a GlobalState::size field that is only written and then >> > migrated but never read from what I can tell. :? >> >> Grrr. It should be used, but it is not :-( >> >> What we have here: >> - A static buffer >> >> uint8_t runstate[100]; >> >> That is partially filled. >> size: is the size of that buffer that is filled. >> >> But, as we are using >> >> VMSTATE_BUFFER(runstate, GlobalState), >> >> We are always sending/receiving the whole buffer. THat is why we have >> trouble with padding. What should we being doing? >> >> Sending just the size, the filled bytes, and making sure that there is >> enough space on destination. >> >> But we aren't donig that. And at this point, I think that I am only >> going to fix the 1st part (always zero pad everything sent). >> >> For fixing the other bit, I need to do an incompatible change. >> >> > Juan: Please clarify the above. Thanks! >> >> Thanks a lot. >> >> Later, Juan. >> >> PD: Why is it done this way? >> Because at the moment, the problem was that qcow2 (as a system, not >> as a device) didn't have a place where to plug pending requests. So >> I created this section that always exist, and anything that has not >> a device associated can hang a subsection here. Once that I created >> it, nobody used it. >> And now, just seing what you are telling, I didn't even used the >> right approach. > > Great, thanks for looking into this. > > Could you base your patches on top of this series? Then you can send > them all together in a single pull request. That way we can be sure > that padding will be added even after switching to pstrcpy() in my > patch. Sure thing. Thanks, Juan.
diff --git a/migration/global_state.c b/migration/global_state.c index 25311479a4..cbe07f21a8 100644 --- a/migration/global_state.c +++ b/migration/global_state.c @@ -44,8 +44,8 @@ void global_state_store_running(void) { const char *state = RunState_str(RUN_STATE_RUNNING); assert(strlen(state) < sizeof(global_state.runstate)); - strncpy((char *)global_state.runstate, - state, sizeof(global_state.runstate)); + pstrcpy((char *)global_state.runstate, + sizeof(global_state.runstate), state); } bool global_state_received(void)
gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) with sanitizers enabled reports the following error: CC migration/global_state.o In file included from /usr/include/string.h:495, from /home/stefanha/qemu/include/qemu/osdep.h:101, from migration/global_state.c:13: In function ‘strncpy’, inlined from ‘global_state_store_running’ at migration/global_state.c:47:5: /usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 100 equals destination size [-Werror=stringop-truncation] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Use pstrcpy() instead of strncpy(). It is guaranteed to NUL-terminate strings. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- migration/global_state.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)