unit names with _{a-f}{a-f} in them cause dbus to crash #49

Merged
merged 1 commit into from Jun 28, 2014

Projects

None yet

5 participants

@jonboulle
Member

Fun and profit.

go program to reproduce:

package main

import (
    "fmt"
    "io/ioutil"
    "os"
    "strings"

    "github.com/coreos/go-systemd/dbus"
)

const contents = `[Service]
ExecStart=/bin/true
`

func log(f string, v ...interface{}) {
    msg := fmt.Sprintf(f, v...)
    if !strings.HasSuffix(msg, "\n") {
        msg += "\n"
    }
    fmt.Printf(msg)
}
func die(f string, v ...interface{}) {
    fmt.Fprintf(os.Stderr, f, v...)
    os.Exit(1)
}

func main() {
    if len(os.Args) != 2 {
        die("usage: %s UNIT_NAME\n", os.Args[0])
    }
    name := os.Args[1]
    path := "/tmp/" + name
    sd, err := dbus.New()
    if err != nil {
        die("error initializing dbus connection: %v\n", err)
    } else {
        log("initialized dbus connection")
    }

    if err := ioutil.WriteFile(path, []byte(contents), os.FileMode(0644)); err != nil {
        die("error writing unit: %v\n", err)
    } else {
        log("wrote unit to disk")
    }

    if _, err = sd.LinkUnitFiles([]string{path}, true, true); err != nil {
        die("error linking unit file: %v\n", err)
    } else {
        log("linked runtime unit file")
    }

    if err := sd.Reload(); err != nil {
        die("error reloading systemd: %v\n", err)
    } else {
        log("reloaded systemd")
    }

    if info, err := sd.GetUnitProperties(name); err != nil {
        die("error getting unit properties: %v\n", err)
    } else {
        log("unit properties: %v\n", info)
    }
    os.Exit(0)
}

script to test cases:

#!/bin/bash

trap "exit" INT

pkill dbus-daemon
for i in - _ a b c d e f g h i j k l m n o p q r s t u w x y z; do 
  for j in - _ a b c d e f g h i j k l m n o p q r s t u w x y z; do 
     name=_${i}${j}.service
     ./go-systemd ${name} &> /dev/null
     if [ $? -ne 0 ]; then
       echo "$name failed!"
       pkill dbus-daemon
       sleep 0.1
     fi
  done
done

results:

core-01 core # ./break.sh 
_aa.service failed!
_ab.service failed!
_ac.service failed!
_ad.service failed!
_ae.service failed!
_af.service failed!
_ba.service failed!
_bb.service failed!
_bc.service failed!
_bd.service failed!
_be.service failed!
_bf.service failed!
_ca.service failed!
_cb.service failed!
_cc.service failed!
_cd.service failed!
_ce.service failed!
_cf.service failed!
_da.service failed!
_db.service failed!
_dc.service failed!
_dd.service failed!
_de.service failed!
_df.service failed!
_ea.service failed!
_eb.service failed!
_ec.service failed!
_ed.service failed!
_ee.service failed!
_ef.service failed!
_fa.service failed!
_fb.service failed!
_fc.service failed!
_fd.service failed!
_fe.service failed!
_ff.service failed!
@marineam
Member

WTF

@jonboulle
Member

Looks like it, those rules aren't sufficient. Still bizarre that dbus gets DOSed

@bcwaldon
Contributor

Yeah, I remember this rabbit hole now. Sorry for not handling it fully in the first place.

On Jun 25, 2014, at 6:53 PM, Jonathan Boulle notifications@github.com wrote:

Looks like it, those rules aren't sufficient. Still bizarre that dbus gets DOSed


Reply to this email directly or view it on GitHub.

@jonboulle
Member

#13

@crawford
Member

Doesn't this just need to escape the escape character?

path = strings.Replace(path, "_", "_5f", -1)
@jonboulle
Member

That'll fix the immediate issue but it's probably better to escape everything that systemd does so we don't just end up here again

@marineam
Member

Yeah, we need to mimic systemctl exactly to avoid this sort of issue.

@jonboulle
Member

ping

@philips
Member
philips commented Jun 28, 2014

lgtm, thank you!

@jonboulle jonboulle merged commit d6fe9c4 into coreos:master Jun 28, 2014
@dharmamike dharmamike added a commit to CenturyLinkLabs/panamax-public-templates that referenced this pull request Aug 21, 2014
@dharmamike dharmamike switching images to avoid issues around systemd unit name issue coreo… d6a8a36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment