| Message ID | 20210818083842.31778-3-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 18/08/2021 09:38, Umang Jain wrote: > IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket > payload. However, if IPCMessage is constructor with one of the s/constructor/constructed/ > following constructors - > > IPCMessage::IPCMessage(), > IPCMessage::IPCMessage(uint32_t cmd) > IPCMessage::IPCMessage(const Header &header) Different spacing on those lines, but it doesn't really matter. > The data_ vector of IPCMessage is empty and uninitialised. In that > case, IPCMessage::payload will try to memcpy() empty data_ vector ... to memcpy() an empty_ data vector ... > which can lead to invoking memcpy() with nullptr. Add a non-empty memcpy() with a nullptr parameter, which is then identified by the address sanity checker. > data_ vector check to avoid it. > > The issue is noticed by running a test manually, testing the vimc Is it not noticed by running the unit tests with 'ninja test'? If not - why not - why does it have to be run manually? > IPA code paths in isolated mode. It is only noticed when the test > is compiled with -Db_sanitize=address,undefined meson built-in option. > > ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/ipc_pipe.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp > index 28e20e03..c8761320 100644 > --- a/src/libcamera/ipc_pipe.cpp > +++ b/src/libcamera/ipc_pipe.cpp > @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const > > memcpy(payload.data.data(), &header_, sizeof(Header)); > > - /* \todo Make this work without copy */ > - memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > + if (data_.size() > 0) { > + /* \todo Make this work without copy */ > + memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > + } Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > payload.fds = fds_; > > return payload; >
Hi Kieran, On 8/18/21 2:26 PM, Kieran Bingham wrote: > Hi Umang, > > On 18/08/2021 09:38, Umang Jain wrote: >> IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket >> payload. However, if IPCMessage is constructor with one of the > s/constructor/constructed/ > >> following constructors - >> >> IPCMessage::IPCMessage(), >> IPCMessage::IPCMessage(uint32_t cmd) >> IPCMessage::IPCMessage(const Header &header) > Different spacing on those lines, but it doesn't really matter. > >> The data_ vector of IPCMessage is empty and uninitialised. In that >> case, IPCMessage::payload will try to memcpy() empty data_ vector > ... to memcpy() an empty_ data vector ... > >> which can lead to invoking memcpy() with nullptr. Add a non-empty > memcpy() with a nullptr parameter, which is then identified by the > address sanity checker. > >> data_ vector check to avoid it. >> >> The issue is noticed by running a test manually, testing the vimc > Is it not noticed by running the unit tests with 'ninja test'? It's not fatal error (in practice), however it "looks" fatal to me. In practice, the test is completed and return with TestFailed / TestPassed values correctly (depending on whether you have locally applied Paul's patch) Currently, the fd leak test shall fail(on master) right? So ninja test will spit out the failure log. You shall see the runtime_error in the failure log, provided you have enabled ASan during meson configure, otherwise not. So, I suspect, the answer to your question is 'it depends' :-) Once the Paul's fix is in with the leak test merged, you won't be able to see the runtime_error. Because the test shall pass via ninja test - and it won't print any log. Hence, it will keep happening(without this series) silently. One might need to check test logs in that case or run the test manually to see the runtime_error caught by ASan. Does that help? Probably not, but this is the state I understand. > > If not - why not - why does it have to be run manually? > > >> IPA code paths in isolated mode. It is only noticed when the test >> is compiled with -Db_sanitize=address,undefined meson built-in option. >> >> ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/libcamera/ipc_pipe.cpp | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp >> index 28e20e03..c8761320 100644 >> --- a/src/libcamera/ipc_pipe.cpp >> +++ b/src/libcamera/ipc_pipe.cpp >> @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const >> >> memcpy(payload.data.data(), &header_, sizeof(Header)); >> >> - /* \todo Make this work without copy */ >> - memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); >> + if (data_.size() > 0) { >> + /* \todo Make this work without copy */ >> + memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); >> + } > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> + >> payload.fds = fds_; >> >> return payload; >>
Hi Umang, Thank you for the patch. On Wed, Aug 18, 2021 at 09:56:44AM +0100, Kieran Bingham wrote: > On 18/08/2021 09:38, Umang Jain wrote: > > IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket > > payload. However, if IPCMessage is constructor with one of the > > s/constructor/constructed/ > > > following constructors - > > > > IPCMessage::IPCMessage(), > > IPCMessage::IPCMessage(uint32_t cmd) > > IPCMessage::IPCMessage(const Header &header) > > Different spacing on those lines, but it doesn't really matter. > > > The data_ vector of IPCMessage is empty and uninitialised. In that > > case, IPCMessage::payload will try to memcpy() empty data_ vector > > ... to memcpy() an empty_ data vector ... > > > which can lead to invoking memcpy() with nullptr. Add a non-empty > > memcpy() with a nullptr parameter, which is then identified by the > address sanity checker. > > > data_ vector check to avoid it. > > > > The issue is noticed by running a test manually, testing the vimc > > Is it not noticed by running the unit tests with 'ninja test'? > > If not - why not - why does it have to be run manually? > > > IPA code paths in isolated mode. It is only noticed when the test > > is compiled with -Db_sanitize=address,undefined meson built-in option. > > > > ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/libcamera/ipc_pipe.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp > > index 28e20e03..c8761320 100644 > > --- a/src/libcamera/ipc_pipe.cpp > > +++ b/src/libcamera/ipc_pipe.cpp > > @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const > > > > memcpy(payload.data.data(), &header_, sizeof(Header)); > > > > - /* \todo Make this work without copy */ > > - memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > > + if (data_.size() > 0) { > > + /* \todo Make this work without copy */ > > + memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); I'd wrap this as memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > > + } > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + > > payload.fds = fds_; > > > > return payload; > >
Hi Umang, On Wed, Aug 18, 2021 at 02:08:42PM +0530, Umang Jain wrote: > IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket > payload. However, if IPCMessage is constructor with one of the > following constructors - > > IPCMessage::IPCMessage(), > IPCMessage::IPCMessage(uint32_t cmd) > IPCMessage::IPCMessage(const Header &header) > > The data_ vector of IPCMessage is empty and uninitialised. In that > case, IPCMessage::payload will try to memcpy() empty data_ vector > which can lead to invoking memcpy() with nullptr. Add a non-empty > data_ vector check to avoid it. > > The issue is noticed by running a test manually, testing the vimc > IPA code paths in isolated mode. It is only noticed when the test > is compiled with -Db_sanitize=address,undefined meson built-in option. > > ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/ipc_pipe.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp > index 28e20e03..c8761320 100644 > --- a/src/libcamera/ipc_pipe.cpp > +++ b/src/libcamera/ipc_pipe.cpp > @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const > > memcpy(payload.data.data(), &header_, sizeof(Header)); > > - /* \todo Make this work without copy */ > - memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > + if (data_.size() > 0) { > + /* \todo Make this work without copy */ > + memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); > + } > + > payload.fds = fds_; > > return payload; > -- > 2.31.0 >
diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp index 28e20e03..c8761320 100644 --- a/src/libcamera/ipc_pipe.cpp +++ b/src/libcamera/ipc_pipe.cpp @@ -102,8 +102,11 @@ IPCUnixSocket::Payload IPCMessage::payload() const memcpy(payload.data.data(), &header_, sizeof(Header)); - /* \todo Make this work without copy */ - memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); + if (data_.size() > 0) { + /* \todo Make this work without copy */ + memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size()); + } + payload.fds = fds_; return payload;
IPCMessage::payload() converts the IPCMessage into an IPCUnixSocket payload. However, if IPCMessage is constructor with one of the following constructors - IPCMessage::IPCMessage(), IPCMessage::IPCMessage(uint32_t cmd) IPCMessage::IPCMessage(const Header &header) The data_ vector of IPCMessage is empty and uninitialised. In that case, IPCMessage::payload will try to memcpy() empty data_ vector which can lead to invoking memcpy() with nullptr. Add a non-empty data_ vector check to avoid it. The issue is noticed by running a test manually, testing the vimc IPA code paths in isolated mode. It is only noticed when the test is compiled with -Db_sanitize=address,undefined meson built-in option. ipc_pipe.cpp:110:8: runtime error: null pointer passed as argument 2, which is declared to never be null Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/ipc_pipe.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)