nsinstall chokes on double slashes in path

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
3 months ago

People

(Reporter: fta+bugzilla, Assigned: fta+bugzilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7) Gecko/2007081520 GranParadiso/3.0a7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7) Gecko/2007081520 GranParadiso/3.0a7

nsinstall is not able to install in paths like /a/b//c/d
if /a/b/ does not already exist. Problem is in mkdirs() which inserts a '\0' preventing 'c' and 'd' to be created.

This has been found while building GranParadiso Alpha7 and trunk on Ubuntu.

Here is what failed:
nsinstall -D /src/firefox-granparadiso-3.0~alpha7/debian/tmp//usr/lib/firefox-granparadiso

nothing after debian/tmp is created.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



patch follows
(Assignee)

Comment 1

12 years ago
Created attachment 277231 [details] [diff] [review]
fix multiple slashes bug

patching both mozilla/nsprpub/config/nsinstall.c and mozilla/config/nsinstall.c

Comment 2

12 years ago
Confirmed ... the patch looks good.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
Attachment #277231 - Flags: review?(benjamin)
(Assignee)

Comment 3

12 years ago
Created attachment 278285 [details] [diff] [review]
fix multiple slashes bug, rev2

Add fix for a 3rd file (security/coreconf/nsinstall/nsinstall.c) causing FTBS in xulrunner on Ubuntu when using cdbs.
Fix an issue with install -D foo (no slash at all) introduced by previous patch.
Attachment #277231 - Attachment is obsolete: true
Attachment #277231 - Flags: review?(benjamin)
(Assignee)

Updated

12 years ago
Attachment #278285 - Flags: review?(benjamin)

Updated

12 years ago
Summary: nsintall and double slashes → nsinstall chokes on double slashes in path

Comment 4

12 years ago
Comment on attachment 278285 [details] [diff] [review]
fix multiple slashes bug, rev2

r=me for the config/nsinstall.c version, which I will land. The NSPR and NSS build systems are independent, and you'll need to get wtc/nelson to review and land those.
Attachment #278285 - Flags: review?(benjamin) → review+

Comment 5

12 years ago
Comment on attachment 278285 [details] [diff] [review]
fix multiple slashes bug, rev2

r=me for the config/nsinstall.c version, which I will land. The NSPR and NSS build systems are independent, and you'll need to get wtc/nelson to review and land those.

Comment 6

12 years ago
Checked in config/nsinstall.c Leaving this bug open for the NSPR and NSS versions.
Assignee: nobody → fta+bugzilla
Comment on attachment 278285 [details] [diff] [review]
fix multiple slashes bug, rev2

requesting wtc's review for the nspr portion of the patch.
Attachment #278285 - Flags: review?(wtc)
Checked in on trunk for coreconf (NSS)
Checking in nsinstall.c; new revision: 1.9; previous revision: 1.8

Notice: this fix will affect all platforms except Windows and OS/2.
(Does anyone know where the source for the Windows+OS/2 version is?)

Comment 9

12 years ago
Comment on attachment 278285 [details] [diff] [review]
fix multiple slashes bug, rev2

r=wtc.  Thanks for the patch.

Please change the first line of the file to:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
                            ^^^                                       ^^^

The for loop can be rewritten as:

    if ((cp = strrchr(path, '/')))
        while (cp != path && cp[-1] == '/')
            cp--;

Case 1: /a/b//c/d

mkdirs("/a/b//c/d", mode)
  mkdirs("/a/b//c", mode)
    mkdirs("/a/b", mode)
      mkdirs("/a", mode)
        mkdir("/a", mode)
      mkdir("/a/b", mode)
    mkdir("/a/b//c", mode)
  mkdir("/a/b//c/d", mode)

Case 2: /a/b/c/  (trailing /)

mkdirs("/a/b/c/", mode)
  mkdirs("/a/b/c", mode)
    mkdirs("/a/b", mode)
      mkdirs("/a", mode)
        mkdir("/a", mode)
      mkdir("/a/b", mode)
    mkdir("/a/b/c", mode)
  mkdir("/a/b/c/", mode)  <== EEXIST

Fortunately we ignore EEXIST, originally for a different
reason though.
Attachment #278285 - Flags: review?(wtc) → review+

Comment 10

12 years ago
I checked in the patch on the NSPR trunk (NSPR 4.7).

I made minor changes to the for loop:

    for (cp = strrchr(path, '/'); cp && cp != path && cp[-1] == '/'; cp--)
        ;

Checking in nsinstall.c;
/cvsroot/mozilla/nsprpub/config/nsinstall.c,v  <--  nsinstall.c
new revision: 3.22; previous revision: 3.21
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.