Re: Bug#557785: AW: Bug#557785: vzctl: symlinked config file: symlink overwrittenwhen --save isspecified
Hi Adam
Sorry for the delay. Here is the debdiff you have asked for.
It is a simple backport of the change done in upstream GIT.
Best regards,
// Ola
On Sun, Dec 13, 2009 at 03:21:59PM +0000, Adam D. Barratt wrote:
> [CC list trimmed]
>
> Hi,
>
> On Sun, 2009-11-29 at 12:30 +0100, Ola Lundqvist wrote:
> > > A)
> > >
> > > * Fix in stable
> > > (This will probably not break any existing configurations, it will just keep links that were removed before)
> > >
> > > Pro: would help fast
> > > Pro: no side effetcs
> > > Con: does it break anything ?
> >
> > Would be a good thing if stable release team accept this kind of update.
> > Added:
> > Con: maybe side effects with 3.0.22 ? Likely not. Just as below.
>
> Please could you provide a debdiff for your proposed change?
>
> Regards,
>
> Adam
>
>
>
--
--------------------- Ola Lundqvist ---------------------------
/ opal@debian.org Annebergsslingan 37 \
| ola@inguza.com 654 65 KARLSTAD |
| http://inguza.com/ +46 (0)70-332 1551 |
\ gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9 /
---------------------------------------------------------------
diff -u vzctl-3.0.22/debian/changelog vzctl-3.0.22/debian/changelog
--- vzctl-3.0.22/debian/changelog
+++ vzctl-3.0.22/debian/changelog
@@ -1,3 +1,11 @@
+vzctl (3.0.22-14lenny1) unstable; urgency=low
+
+ * Correction for destroyed symlink for configuration file backported from
+ upstream GIT. This solves a potential data corruption issue as described
+ in the bug report. Closes: #557785.
+
+ -- Ola Lundqvist <opal@debian.org> Sun, 20 Dec 2009 19:33:36 +0100
+
vzctl (3.0.22-14) unstable; urgency=medium
* Correction to make it work with ipv6, closes: #505792. Thanks to
only in patch2:
unchanged:
--- vzctl-3.0.22.orig/src/lib/config.c
+++ vzctl-3.0.22/src/lib/config.c
@@ -15,6 +15,9 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
@@ -2166,43 +2169,56 @@
static int write_conf(char *fname, list_head_t *head)
{
- char buf[STR_SIZE];
+ char *tmpfile, *file;
conf_struct *conf;
- int fd = 2;
- int len, ret;
-
- if (fname != NULL) {
- snprintf(buf, sizeof(buf), "%s.tmp", fname);
- if ((fd = open(buf, O_CREAT|O_WRONLY|O_TRUNC, 0644)) < 0) {
- logger(-1, errno, "Unable to create configuration"
- " file %s", buf);
+ FILE * fp;
+ int ret = 1;
+ const char *suffix = ".tmp";
+ char *fmt;
+
+ file = canonicalize_file_name(fname);
+ if (file == NULL) {
+ if (errno != ENOENT)
+ {
+ logger(-1, errno, "Unable to resolve path %s", fname);
return 1;
}
+ file = strdup(fname);
+ }
+ tmpfile = malloc(strlen(file) + strlen(suffix) + 1);
+ sprintf(tmpfile, "%s%s", file, suffix);
+ if ((fp = fopen(tmpfile, "w")) == NULL) {
+ logger(-1, errno, "Unable to create configuration"
+ " file %s", tmpfile);
+ goto out;
}
list_for_each(conf, head, list) {
if (conf->val == NULL)
continue;
- len = strlen(conf->val);
- ret = write(fd, conf->val, len);
- if (ret < 0) {
- logger(-1, errno, "Unable to write %d bytes to %s",
- len, buf);
- unlink(buf);
- close(fd);
- return 1;
- }
if (strchr(conf->val, '\n') == NULL)
- write(fd, "\n", 1);
- }
- if (fname != NULL) {
- close(fd);
- if (rename(buf, fname)) {
- logger(-1, errno, "Unable to move %s -> %s",
- buf, fname);
- return 1;
+ fmt="%s\n";
+ else
+ fmt="%s";
+ if (fprintf(fp, fmt, conf->val) < 0) {
+ logger(-1, errno, "Error writing to %s",
+ tmpfile);
+ fclose(fp);
+ goto out2;
}
}
- return 0;
+ fclose(fp);
+ if (rename(tmpfile, file)) {
+ logger(-1, errno, "Unable to move %s -> %s",
+ tmpfile, file );
+ goto out2;
+ }
+ ret = 0;
+out2:
+ unlink(tmpfile);
+out:
+ free(tmpfile);
+ free(file);
+ return ret;
}
static int vps_merge_conf(list_head_t *dst, list_head_t *src)
only in patch2:
unchanged:
--- vzctl-3.0.22.orig/debian/patches/config-symlink-corr.patch
+++ vzctl-3.0.22/debian/patches/config-symlink-corr.patch
@@ -0,0 +1,133 @@
+From: Kir Kolyshkin <kir@openvz.org>
+Date: Fri, 16 Oct 2009 13:05:29 +0000 (+0400)
+Subject: vzctl: refactor write_conf()
+X-Git-Url: http://git.openvz.org/?p=vzctl;a=commitdiff_plain;h=35c8a3e3c963446e98b087ea629f32647512af25;hp=acb231d37f8581a31ab98be8deb499e0828f2012
+
+vzctl: refactor write_conf()
+
+There were a few bugs:
+1. If CT config file was a symlink pointing to a file residing
+ on another partition, we failed miserably (bug #1270). To fix,
+ use canonicalize_file_name. Bug reported by xavier martin.
+
+2. Return value from write() syscall was not checked in one place.
+
+In addition to fixing the above bugs, the following improvements were made:
+1. Buffer for tmpfile is allocated dynamically.
+2. Get rid of functionality of writing to stderr, it was never used
+ (maybe only for debug, but it's prehistorical).
+3. Switched from open/write/close to fopen/fprintf/fclose.
+4. Saved (potentially) one syscall for writing '\n'.
+
+Hope I haven't added more bugs...
+
+NOTE1: check (strchr(conf->val, '\n') == NULL) is sufficient here,
+since those lines are read by fgets() in read_conf(), so they can
+only contain '\n' in the end, not in the middle.
+
+NOTE2: we still do not check fprintf() result properly, since it may
+end up writing only part of given string. Not sure if it can hit or not.
+
+http://bugzilla.openvz.org/1270
+
+Signed-off-by: Kir Kolyshkin <kir@openvz.org>
+---
+
+diff --git a/src/lib/config.c b/src/lib/config.c
+index 4c579d7..de8ac09 100644
+--- a/src/lib/config.c
++++ b/src/lib/config.c
+@@ -15,6 +15,9 @@
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
++#ifndef _GNU_SOURCE
++#define _GNU_SOURCE
++#endif
+ #include <stdlib.h>
+ #include <string.h>
+ #include <ctype.h>
+@@ -2290,43 +2293,56 @@ static int read_conf(char *fname, list_head_t *conf_h)
+
+ static int write_conf(char *fname, list_head_t *head)
+ {
+- char buf[STR_SIZE];
++ char *tmpfile, *file;
+ conf_struct *conf;
+- int fd = 2;
+- int len, ret;
+-
+- if (fname != NULL) {
+- snprintf(buf, sizeof(buf), "%s.tmp", fname);
+- if ((fd = open(buf, O_CREAT|O_WRONLY|O_TRUNC, 0644)) < 0) {
+- logger(-1, errno, "Unable to create configuration"
+- " file %s", buf);
++ FILE * fp;
++ int ret = 1;
++ const char *suffix = ".tmp";
++ char *fmt;
++
++ file = canonicalize_file_name(fname);
++ if (file == NULL) {
++ if (errno != ENOENT)
++ {
++ logger(-1, errno, "Unable to resolve path %s", fname);
+ return 1;
+ }
++ file = strdup(fname);
++ }
++ tmpfile = malloc(strlen(file) + strlen(suffix) + 1);
++ sprintf(tmpfile, "%s%s", file, suffix);
++ if ((fp = fopen(tmpfile, "w")) == NULL) {
++ logger(-1, errno, "Unable to create configuration"
++ " file %s", tmpfile);
++ goto out;
+ }
+ list_for_each(conf, head, list) {
+ if (conf->val == NULL)
+ continue;
+- len = strlen(conf->val);
+- ret = write(fd, conf->val, len);
+- if (ret < 0) {
+- logger(-1, errno, "Unable to write %d bytes to %s",
+- len, buf);
+- unlink(buf);
+- close(fd);
+- return 1;
+- }
+ if (strchr(conf->val, '\n') == NULL)
+- write(fd, "\n", 1);
+- }
+- if (fname != NULL) {
+- close(fd);
+- if (rename(buf, fname)) {
+- logger(-1, errno, "Unable to move %s -> %s",
+- buf, fname);
+- return 1;
++ fmt="%s\n";
++ else
++ fmt="%s";
++ if (fprintf(fp, fmt, conf->val) < 0) {
++ logger(-1, errno, "Error writing to %s",
++ tmpfile);
++ fclose(fp);
++ goto out2;
+ }
+ }
+- return 0;
++ fclose(fp);
++ if (rename(tmpfile, file)) {
++ logger(-1, errno, "Unable to move %s -> %s",
++ tmpfile, file );
++ goto out2;
++ }
++ ret = 0;
++out2:
++ unlink(tmpfile);
++out:
++ free(tmpfile);
++ free(file);
++ return ret;
+ }
+
+ static int vps_merge_conf(list_head_t *dst, list_head_t *src)
Reply to: