Code review comment for lp:lava-dashboard/multinode

Revision history for this message
Antonio Terceiro (terceiro) wrote :

> === modified file 'dashboard_app/xmlrpc.py'
> --- dashboard_app/xmlrpc.py 2013-05-02 10:35:29 +0000
> +++ dashboard_app/xmlrpc.py 2013-08-20 16:56:49 +0000
> @@ -2,7 +2,7 @@
> #
> # Author: Zygmunt Krynicki <email address hidden>
> #
> -# This file is part of Launch Control.
> +# This file is part of LAVA Dashboard
> #
> # Launch Control is free software: you can redistribute it and/or modify
> # it under the terms of the GNU Affero General Public License version 3
> @@ -25,7 +25,9 @@
> import logging
> import re
> import xmlrpclib
> -
> +import hashlib
> +import json
> +import os
> from django.contrib.auth.models import User, Group
> from django.core.urlresolvers import reverse
> from django.db import IntegrityError, DatabaseError
> @@ -243,6 +245,152 @@
> 'dashboard_app.views.redirect_to_bundle',
> kwargs={'content_sha1':bundle.content_sha1}))
>
> + def put_pending(self, content, group_name):
> + """
[...]
> + """
> + try:
> + # add this to a list which put_group can use.
> + sha1 = hashlib.sha1()
> + sha1.update(content)
> + hexdigest = sha1.hexdigest()
> + groupfile = "/tmp/%s" % group_name
> + with open(groupfile, "a+") as grp_file:
> + grp_file.write("%s\n" % content)
> + return hexdigest
> + except Exception as e:
> + logging.debug("Dashboard pending submission caused an exception: %s" % e)

Is there a race condition here? It's fine for two or more processes to append
to the same file, but it's possible that depending on the size of the bundles
and on line buffering issues the contents of different bundles might get
intermingled. Maybe we should write each bundle to its own separate file, then
read them all on put_group.

Also, I miss some sort of authentication to avoid the risk of having attackers
submitting random crap into bundle streams for multinode groups. I guess
put_group already handles authentication because it uses the underlying bundle
stream access control?

« Back to merge proposal