[PATCH 08/10] Add support for MIT-SHM AttachFd request

Kristian Høgsberg krh at bitplanet.net
Fri Nov 1 06:17:40 CET 2013


On Thu, Oct 31, 2013 at 3:43 PM, Keith Packard <keithp at keithp.com> wrote:
> This passes a file descriptor from the client to the server, which is
> then mmap'd

A problem we recently hit in wayland, which also affects this
extension is that a client can set up shared memory like this and the
truncate the tmp file to 0.  When the server then tries to access the
mapped memory it dies with SIGBUS.  We're planning on handling this
case by installing a SIGBUS handler that flags the error, maps
/dev/zero over the faulting mmap area and then lets the access
continue.  We'll wrap access the the map with call to begin/end access
functions and in the end_access function we check the flag to see if
the access cause a fault and kill the client in that case.

Kristian

> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  Xext/shm.c    | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  Xext/shmint.h |   1 +
>  include/os.h  |   2 +
>  os/io.c       |   8 +++
>  4 files changed, 167 insertions(+), 3 deletions(-)
>
> diff --git a/Xext/shm.c b/Xext/shm.c
> index f6995cc..1a70260 100644
> --- a/Xext/shm.c
> +++ b/Xext/shm.c
> @@ -53,6 +53,7 @@ in this Software without prior written authorization from The Open Group.
>  #include "xace.h"
>  #include <X11/extensions/shmproto.h>
>  #include <X11/Xfuncproto.h>
> +#include <sys/mman.h>
>  #include "protocol-versions.h"
>
>  /* Needed for Solaris cross-zone shared memory extension */
> @@ -382,8 +383,10 @@ ProcShmAttach(ClientPtr client)
>          client->errorValue = stuff->readOnly;
>          return BadValue;
>      }
> -    for (shmdesc = Shmsegs;
> -         shmdesc && (shmdesc->shmid != stuff->shmid); shmdesc = shmdesc->next);
> +    for (shmdesc = Shmsegs; shmdesc; shmdesc = shmdesc->next) {
> +        if (!shmdesc->is_fd && shmdesc->shmid == stuff->shmid)
> +            break;
> +    }
>      if (shmdesc) {
>          if (!stuff->readOnly && !shmdesc->writable)
>              return BadAccess;
> @@ -393,6 +396,7 @@ ProcShmAttach(ClientPtr client)
>          shmdesc = malloc(sizeof(ShmDescRec));
>          if (!shmdesc)
>              return BadAlloc;
> +        shmdesc->is_fd = FALSE;
>          shmdesc->addr = shmat(stuff->shmid, 0,
>                                stuff->readOnly ? SHM_RDONLY : 0);
>          if ((shmdesc->addr == ((char *) -1)) || SHMSTAT(stuff->shmid, &buf)) {
> @@ -431,7 +435,10 @@ ShmDetachSegment(pointer value, /* must conform to DeleteType */
>
>      if (--shmdesc->refcnt)
>          return TRUE;
> -    shmdt(shmdesc->addr);
> +    if (shmdesc->is_fd)
> +        munmap(shmdesc->addr, shmdesc->size);
> +    else
> +        shmdt(shmdesc->addr);
>      for (prev = &Shmsegs; *prev != shmdesc; prev = &(*prev)->next);
>      *prev = shmdesc->next;
>      free(shmdesc);
> @@ -1088,6 +1095,122 @@ ProcShmCreatePixmap(ClientPtr client)
>  }
>
>  static int
> +ProcShmAttachFd(ClientPtr client)
> +{
> +    int fd;
> +    ShmDescPtr shmdesc;
> +    REQUEST(xShmAttachFdReq);
> +    struct stat statb;
> +
> +    SetReqFds(client, 1);
> +    REQUEST_SIZE_MATCH(xShmAttachFdReq);
> +    LEGAL_NEW_RESOURCE(stuff->shmseg, client);
> +    if ((stuff->readOnly != xTrue) && (stuff->readOnly != xFalse)) {
> +        client->errorValue = stuff->readOnly;
> +        return BadValue;
> +    }
> +    fd = ReadFdFromClient(client);
> +    if (fd < 0)
> +        return BadMatch;
> +
> +    if (fstat(fd, &statb) < 0 || statb.st_size == 0) {
> +        close(fd);
> +        return BadMatch;
> +    }
> +
> +    shmdesc = malloc(sizeof(ShmDescRec));
> +    if (!shmdesc) {
> +        close(fd);
> +        return BadAlloc;
> +    }
> +    shmdesc->is_fd = TRUE;
> +    shmdesc->addr = mmap(NULL, statb.st_size,
> +                         stuff->readOnly ? PROT_READ : PROT_READ|PROT_WRITE,
> +                         MAP_SHARED,
> +                         fd, 0);
> +
> +    close(fd);
> +    if ((shmdesc->addr == ((char *) -1))) {
> +        free(shmdesc);
> +        return BadAccess;
> +    }
> +
> +    shmdesc->refcnt = 1;
> +    shmdesc->writable = !stuff->readOnly;
> +    shmdesc->size = statb.st_size;
> +    shmdesc->next = Shmsegs;
> +    Shmsegs = shmdesc;
> +
> +    if (!AddResource(stuff->shmseg, ShmSegType, (pointer) shmdesc))
> +        return BadAlloc;
> +    return Success;
> +}
> +
> +static int
> +ProcShmCreateSegment(ClientPtr client)
> +{
> +    int fd;
> +    ShmDescPtr shmdesc;
> +    REQUEST(xShmCreateSegmentReq);
> +    xShmCreateSegmentReply rep = {
> +        .type = X_Reply,
> +        .nfd = 1,
> +        .sequenceNumber = client->sequence,
> +        .length = 0,
> +    };
> +    char template[] = "/tmp/shm-XXXXXX";
> +
> +    REQUEST_SIZE_MATCH(xShmCreateSegmentReq);
> +    if ((stuff->readOnly != xTrue) && (stuff->readOnly != xFalse)) {
> +        client->errorValue = stuff->readOnly;
> +        return BadValue;
> +    }
> +    fd = mkstemp(template);
> +    if (fd < 0)
> +        return BadAlloc;
> +    unlink(template);
> +    if (ftruncate(fd, stuff->size) < 0) {
> +        close(fd);
> +        return BadAlloc;
> +    }
> +    shmdesc = malloc(sizeof(ShmDescRec));
> +    if (!shmdesc) {
> +        close(fd);
> +        return BadAlloc;
> +    }
> +    shmdesc->is_fd = TRUE;
> +    shmdesc->addr = mmap(NULL, stuff->size,
> +                         stuff->readOnly ? PROT_READ : PROT_READ|PROT_WRITE,
> +                         MAP_SHARED,
> +                         fd, 0);
> +
> +    if ((shmdesc->addr == ((char *) -1))) {
> +        close(fd);
> +        free(shmdesc);
> +        return BadAccess;
> +    }
> +
> +    shmdesc->refcnt = 1;
> +    shmdesc->writable = !stuff->readOnly;
> +    shmdesc->size = stuff->size;
> +    shmdesc->next = Shmsegs;
> +    Shmsegs = shmdesc;
> +
> +    if (!AddResource(stuff->shmseg, ShmSegType, (pointer) shmdesc)) {
> +        close(fd);
> +        return BadAlloc;
> +    }
> +
> +    if (WriteFdToClient(client, fd, TRUE) < 0) {
> +        FreeResource(stuff->shmseg, RT_NONE);
> +        close(fd);
> +        return BadAlloc;
> +    }
> +    WriteToClient(client, sizeof (xShmCreateSegmentReply), &rep);
> +    return Success;
> +}
> +
> +static int
>  ProcShmDispatch(ClientPtr client)
>  {
>      REQUEST(xReq);
> @@ -1116,6 +1239,10 @@ ProcShmDispatch(ClientPtr client)
>              return ProcPanoramiXShmCreatePixmap(client);
>  #endif
>          return ProcShmCreatePixmap(client);
> +    case X_ShmAttachFd:
> +        return ProcShmAttachFd(client);
> +    case X_ShmCreateSegment:
> +        return ProcShmCreateSegment(client);
>      default:
>          return BadRequest;
>      }
> @@ -1217,6 +1344,28 @@ SProcShmCreatePixmap(ClientPtr client)
>  }
>
>  static int
> +SProcShmAttachFd(ClientPtr client)
> +{
> +    REQUEST(xShmAttachFdReq);
> +    SetReqFds(client, 1);
> +    swaps(&stuff->length);
> +    REQUEST_SIZE_MATCH(xShmAttachFdReq);
> +    swapl(&stuff->shmseg);
> +    return ProcShmAttachFd(client);
> +}
> +
> +static int
> +SProcShmCreateSegment(ClientPtr client)
> +{
> +    REQUEST(xShmCreateSegmentReq);
> +    swaps(&stuff->length);
> +    REQUEST_SIZE_MATCH(xShmCreateSegmentReq);
> +    swapl(&stuff->shmseg);
> +    swapl(&stuff->size);
> +    return ProcShmCreateSegment(client);
> +}
> +
> +static int
>  SProcShmDispatch(ClientPtr client)
>  {
>      REQUEST(xReq);
> @@ -1233,6 +1382,10 @@ SProcShmDispatch(ClientPtr client)
>          return SProcShmGetImage(client);
>      case X_ShmCreatePixmap:
>          return SProcShmCreatePixmap(client);
> +    case X_ShmAttachFd:
> +        return SProcShmAttachFd(client);
> +    case X_ShmCreateSegment:
> +        return SProcShmCreateSegment(client);
>      default:
>          return BadRequest;
>      }
> diff --git a/Xext/shmint.h b/Xext/shmint.h
> index 9002ce5..db35fbb 100644
> --- a/Xext/shmint.h
> +++ b/Xext/shmint.h
> @@ -61,6 +61,7 @@ typedef struct _ShmDesc {
>      int shmid;
>      int refcnt;
>      char *addr;
> +    Bool is_fd;
>      Bool writable;
>      unsigned long size;
>  } ShmDescRec, *ShmDescPtr;
> diff --git a/include/os.h b/include/os.h
> index b654a0d..11b2198 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -100,6 +100,8 @@ extern _X_EXPORT int ReadRequestFromClient(ClientPtr /*client */ );
>
>  extern _X_EXPORT int ReadFdFromClient(ClientPtr client);
>
> +extern _X_EXPORT int WriteFdToClient(ClientPtr client, int fd, Bool do_close);
> +
>  extern _X_EXPORT Bool InsertFakeRequest(ClientPtr /*client */ ,
>                                          char * /*data */ ,
>                                          int /*count */ );
> diff --git a/os/io.c b/os/io.c
> index 83df6e9..a20faa5 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -506,6 +506,14 @@ ReadFdFromClient(ClientPtr client)
>      return fd;
>  }
>
> +int
> +WriteFdToClient(ClientPtr client, int fd, Bool do_close)
> +{
> +    OsCommPtr oc = (OsCommPtr) client->osPrivate;
> +
> +    return _XSERVTransSendFd(oc->trans_conn, fd, do_close);
> +}
> +
>  /*****************************************************************
>   * InsertFakeRequest
>   *    Splice a consed up (possibly partial) request in as the next request.
> --
> 1.8.4.rc3
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel


More information about the xorg-devel mailing list