Valgrind: r16274 - in /trunk: NEWS coregrind/m_syswrap/syswrap-linux.c include/pub_tool_basics.h

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Valgrind: r16274 - in /trunk: NEWS coregrind/m_syswrap/syswrap-linux.c include/pub_tool_basics.h

svn-2
Author: philippe
Date: Wed Mar 15 19:35:29 2017
New Revision: 16274

Log:
Fix 376956  syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
        to be wrongly marked as addressable

Patch from Daniel Glöckner, slightly modified.


Modified:
    trunk/NEWS
    trunk/coregrind/m_syswrap/syswrap-linux.c
    trunk/include/pub_tool_basics.h

Modified: trunk/NEWS
==============================================================================
--- trunk/NEWS (original)
+++ trunk/NEWS Wed Mar 15 19:35:29 2017
@@ -142,6 +142,8 @@
 376611  ppc64 and arm64 don't know about prlimit64 syscall
 376729  PPC64, remove R2 from the clobber list
         == 371668
+376956  syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
+        to be wrongly marked as addressable
 377427  PPC64, lxv instruction failing on odd destination register
 377478  PPC64: ISA 3.0 setup fixes
 

Modified: trunk/coregrind/m_syswrap/syswrap-linux.c
==============================================================================
--- trunk/coregrind/m_syswrap/syswrap-linux.c (original)
+++ trunk/coregrind/m_syswrap/syswrap-linux.c Wed Mar 15 19:35:29 2017
@@ -6069,6 +6069,11 @@
    ioctl wrappers
    ------------------------------------------------------------------ */
 
+struct vg_drm_version_info {
+   struct vki_drm_version data;
+   struct vki_drm_version *orig; // Original ARG3 pointer value at syscall entry.
+};
+
 PRE(sys_ioctl)
 {
    *flags |= SfMayBlock;
@@ -7686,7 +7691,8 @@
 
    case VKI_DRM_IOCTL_VERSION:
       if (ARG3) {
-         struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
+         struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
+         struct vg_drm_version_info* info;
  PRE_MEM_WRITE("ioctl(DRM_VERSION).version_major", (Addr)&data->version_major, sizeof(data->version_major));
          PRE_MEM_WRITE("ioctl(DRM_VERSION).version_minor", (Addr)&data->version_minor, sizeof(data->version_minor));
          PRE_MEM_WRITE("ioctl(DRM_VERSION).version_patchlevel", (Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
@@ -7699,6 +7705,10 @@
          PRE_MEM_READ("ioctl(DRM_VERSION).desc_len", (Addr)&data->desc_len, sizeof(data->desc_len));
          PRE_MEM_READ("ioctl(DRM_VERSION).desc", (Addr)&data->desc, sizeof(data->desc));
          PRE_MEM_WRITE("ioctl(DRM_VERSION).desc", (Addr)data->desc, data->desc_len);
+         info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
+         info->data = *data;
+         info->orig = data;
+         ARG3 = (Addr)&info->data;
       }
       break;
    case VKI_DRM_IOCTL_GET_UNIQUE:
@@ -10174,16 +10184,24 @@
 
    case VKI_DRM_IOCTL_VERSION:
       if (ARG3) {
-         struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
+         struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
+         struct vg_drm_version_info* info = container_of(data, struct vg_drm_version_info, data);
+         const vki_size_t orig_name_len = info->orig->name_len;
+         const vki_size_t orig_date_len = info->orig->date_len;
+         const vki_size_t orig_desc_len = info->orig->desc_len;
+         *info->orig = info->data;
+         ARG3 = (Addr)info->orig;
+         data = info->orig;
+         VG_(free)(info);
  POST_MEM_WRITE((Addr)&data->version_major, sizeof(data->version_major));
          POST_MEM_WRITE((Addr)&data->version_minor, sizeof(data->version_minor));
          POST_MEM_WRITE((Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
          POST_MEM_WRITE((Addr)&data->name_len, sizeof(data->name_len));
-         POST_MEM_WRITE((Addr)data->name, data->name_len);
+         POST_MEM_WRITE((Addr)data->name, VG_MIN(data->name_len, orig_name_len));
          POST_MEM_WRITE((Addr)&data->date_len, sizeof(data->date_len));
-         POST_MEM_WRITE((Addr)data->date, data->date_len);
+         POST_MEM_WRITE((Addr)data->date, VG_MIN(data->date_len, orig_date_len));
          POST_MEM_WRITE((Addr)&data->desc_len, sizeof(data->desc_len));
-         POST_MEM_WRITE((Addr)data->desc, data->desc_len);
+         POST_MEM_WRITE((Addr)data->desc, VG_MIN(data->desc_len, orig_desc_len));
       }
       break;
    case VKI_DRM_IOCTL_GET_UNIQUE:

Modified: trunk/include/pub_tool_basics.h
==============================================================================
--- trunk/include/pub_tool_basics.h (original)
+++ trunk/include/pub_tool_basics.h Wed Mar 15 19:35:29 2017
@@ -396,6 +396,10 @@
 #   define offsetof(type,memb) ((SizeT)(HWord)&((type*)0)->memb)
 #endif
 
+#if !defined(container_of)
+#   define container_of(ptr, type, member) ((type *)((char *)(ptr) - offsetof(type, member)))
+#endif
+
 /* Alignment */
 /* We use a prefix vg_ for vg_alignof as its behaviour slightly
    differs from the standard alignof/gcc defined __alignof__


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: Valgrind: r16274 - in /trunk: NEWS coregrind/m_syswrap/syswrap-linux.c include/pub_tool_basics.h

iraisr
2017-03-15 20:35 GMT+01:00  <[hidden email]>:

> Author: philippe
> Date: Wed Mar 15 19:35:29 2017
> New Revision: 16274
>
> Log:
> Fix 376956  syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
>         to be wrongly marked as addressable
>
> Patch from Daniel Glöckner, slightly modified.
>
>
> Modified:
>     trunk/NEWS
>     trunk/coregrind/m_syswrap/syswrap-linux.c
>     trunk/include/pub_tool_basics.h
>
> Modified: trunk/NEWS
> ==============================================================================
> --- trunk/NEWS (original)
> +++ trunk/NEWS Wed Mar 15 19:35:29 2017
> @@ -142,6 +142,8 @@
>  376611  ppc64 and arm64 don't know about prlimit64 syscall
>  376729  PPC64, remove R2 from the clobber list
>          == 371668
> +376956  syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
> +        to be wrongly marked as addressable
>  377427  PPC64, lxv instruction failing on odd destination register
>  377478  PPC64: ISA 3.0 setup fixes
>
>
> Modified: trunk/coregrind/m_syswrap/syswrap-linux.c
> ==============================================================================
> --- trunk/coregrind/m_syswrap/syswrap-linux.c (original)
> +++ trunk/coregrind/m_syswrap/syswrap-linux.c Wed Mar 15 19:35:29 2017
> @@ -6069,6 +6069,11 @@
>     ioctl wrappers
>     ------------------------------------------------------------------ */
>
> +struct vg_drm_version_info {
> +   struct vki_drm_version data;
> +   struct vki_drm_version *orig; // Original ARG3 pointer value at syscall entry.
> +};
> +
>  PRE(sys_ioctl)
>  {
>     *flags |= SfMayBlock;
> @@ -7686,7 +7691,8 @@
>
>     case VKI_DRM_IOCTL_VERSION:
>        if (ARG3) {
> -         struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
> +         struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
> +         struct vg_drm_version_info* info;
>          PRE_MEM_WRITE("ioctl(DRM_VERSION).version_major", (Addr)&data->version_major, sizeof(data->version_major));
>           PRE_MEM_WRITE("ioctl(DRM_VERSION).version_minor", (Addr)&data->version_minor, sizeof(data->version_minor));
>           PRE_MEM_WRITE("ioctl(DRM_VERSION).version_patchlevel", (Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
> @@ -7699,6 +7705,10 @@
>           PRE_MEM_READ("ioctl(DRM_VERSION).desc_len", (Addr)&data->desc_len, sizeof(data->desc_len));
>           PRE_MEM_READ("ioctl(DRM_VERSION).desc", (Addr)&data->desc, sizeof(data->desc));
>           PRE_MEM_WRITE("ioctl(DRM_VERSION).desc", (Addr)data->desc, data->desc_len);
> +         info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
> +         info->data = *data;
> +         info->orig = data;
> +         ARG3 = (Addr)&info->data;
>        }
>        break;

Does this create a memory leak if the ioctl fails?
I think it does because POST(sys_ioctl) is called only on success.

I can think of several approaches here:
- have POST(sys_ioctl) called also on failure
- convey the required information in some other way
- leave it as is and document somewhere this could leak some memory

I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: Valgrind: r16274 - in /trunk: NEWS coregrind/m_syswrap/syswrap-linux.c include/pub_tool_basics.h

Philippe Waroquiers
On Wed, 2017-03-15 at 21:28 +0100, Ivo Raisr wrote:
> 2017-03-15 20:35 GMT+01:00  <[hidden email]>:

> > +         info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
> > +         info->data = *data;
> > +         info->orig = data;
> > +         ARG3 = (Addr)&info->data;
> >        }
> >        break;
>
> Does this create a memory leak if the ioctl fails?
> I think it does because POST(sys_ioctl) is called only on success.
>
> I can think of several approaches here:
> - have POST(sys_ioctl) called also on failure
> - convey the required information in some other way
> - leave it as is and document somewhere this could leak some memory
Good catch, yes, I think it would leak.

I guess we might have to put the flag  SfPostOnFail, like
for ppoll and pselect6 ?

And then, in the POST, just execute the POST_MEM_WRITE operations
if success ?
(and always release the memory)

Philippe


 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
Reply | Threaded
Open this post in threaded view
|

Re: Valgrind: r16274 - in /trunk: NEWS coregrind/m_syswrap/syswrap-linux.c include/pub_tool_basics.h

iraisr
2017-03-15 22:56 GMT+01:00 Philippe Waroquiers <[hidden email]>:

> On Wed, 2017-03-15 at 21:28 +0100, Ivo Raisr wrote:
>> 2017-03-15 20:35 GMT+01:00  <[hidden email]>:
>
>> > +         info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
>> > +         info->data = *data;
>> > +         info->orig = data;
>> > +         ARG3 = (Addr)&info->data;
>> >        }
>> >        break;
>>
>> Does this create a memory leak if the ioctl fails?
>> I think it does because POST(sys_ioctl) is called only on success.
>>
>> I can think of several approaches here:
>> - have POST(sys_ioctl) called also on failure
>> - convey the required information in some other way
>> - leave it as is and document somewhere this could leak some memory
>
> Good catch, yes, I think it would leak.
>
> I guess we might have to put the flag  SfPostOnFail, like
> for ppoll and pselect6?
>
> And then, in the POST, just execute the POST_MEM_WRITE operations
> if success?
> (and always release the memory)

Yes, that's one of the options possible.
Thank you for looking at this.
I.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Valgrind-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/valgrind-developers