Merge pull request #5426 from MartinNowak/fix313

fix Issue 313 - Fully qualified names bypass private imports

changelog.dd

@@ -1,10 +1,11 @@
 Ddoc
 
-$(COMMENT Pending changelog for 2.070. This will get copied to dlang.org and
-    cleared when master gets merged into stable prior to 2.069.
+$(COMMENT Pending changelog for 2.071. This will get copied to dlang.org and
+    cleared when master gets merged into stable.
 )
 
 $(BUGSTITLE Compiler Changes,
+    $(LI $(RELATIVE_LINK2 imports-313, Import access checks for fully qualified names were fixed.))
 )
 
 $(BUGSTITLE Language Changes,
@@ -13,7 +14,21 @@ $(BUGSTITLE Language Changes,
 )
 
 $(BUGSTITLE Compiler Changes,
+    $(LI $(LNAME2 imports-313, Import access checks for fully qualified names were fixed.)
 
+    $(P It is no longer possible to bypass private imports by using fully qualified names, e.g.
+        the following example will fail with `package std.range is not accessible here`.
+    )
+
+    ---
+    import std.algorithm;
+
+    static assert(std.range.isForwardRange!string);
+    ---
+
+    $(P To ease updating existing code, the old behavior was retained but deprecated.
+    )
+    )
 )
 
 $(BUGSTITLE Language Changes,

src/access.d

@@ -410,3 +410,34 @@ extern (C++) bool checkAccess(Loc loc, Scope* sc, Expression e, Declaration d)
     }
     return false;
 }
+
+/****************************************
+ * Check access to package/module `p` from scope `sc`.
+ *
+ * Params:
+ *   loc = source location for issued error message
+ *   sc = scope from which to access to a fully qualified package name
+ *   p = the package/module to check access for
+ * Returns: true if the package is not accessible.
+ *
+ * Because a global symbol table tree is used for imported packages/modules,
+ * access to them needs to be checked based on the imports in the scope chain
+ * (see Bugzilla 313).
+ *
+ */
+extern (C++) bool checkAccess(Loc loc, Scope* sc, Package p)
+{
+    if (sc._module == p)
+        return false;
+    for (; sc; sc = sc.enclosing)
+    {
+        if (sc.scopesym && sc.scopesym.isPackageAccessible(p))
+            return false;
+    }
+    auto name = p.toPrettyChars();
+    if (p.isPkgMod == PKGmodule || p.isModule())
+        deprecation(loc, "%s %s is not accessible here, perhaps add 'static import %s;'", p.kind(), name, name);
+    else
+        deprecation(loc, "%s %s is not accessible here", p.kind(), name);
+    return true;
+}

src/dimport.d

@@ -160,6 +160,8 @@ public:
                         {
                             // mod is a package.d, or a normal module which conflicts with the package name.
                             assert(mod.isPackageFile == (p.isPkgMod == PKGmodule));
+                            if (mod.isPackageFile)
+                                mod.tag = p.tag; // reuse the same package tag
                         }
                     }
                     else
@@ -244,19 +246,42 @@ public:
             // Modules need a list of each imported module
             //printf("%s imports %s\n", sc.module.toChars(), mod.toChars());
             sc._module.aimports.push(mod);
-            if (!isstatic && !aliasId && !names.dim)
+
+            if (!aliasId && !names.dim) // neither a selective nor a renamed import
             {
-                if (sc.explicitProtection)
-                    protection = sc.protection.kind;
+                ScopeDsymbol scopesym;
                 for (Scope* scd = sc; scd; scd = scd.enclosing)
                 {
-                    if (scd.scopesym)
+                    if (!scd.scopesym)
+                        continue;
+                    scopesym = scd.scopesym;
+                    break;
+                }
+
+                if (!isstatic)
+                {
+                    if (sc.explicitProtection)
+                        protection = sc.protection.kind;
+                    scopesym.importScope(mod, Prot(protection));
+                }
+
+                // Mark the imported packages as accessible from the current
+                // scope. This access check is necessary when using FQN b/c
+                // we're using a single global package tree. See Bugzilla 313.
+                if (packages)
+                {
+                    // import a.b.c.d;
+                    auto p = pkg; // a
+                    scopesym.addAccessiblePackage(p);
+                    foreach (id; (*packages)[1 .. packages.dim]) // [b, c]
                     {
-                        scd.scopesym.importScope(mod, Prot(protection));
-                        break;
+                        p = cast(Package) p.symtab.lookup(id);
+                        scopesym.addAccessiblePackage(p);
                     }
                 }
+                scopesym.addAccessiblePackage(mod); // d
             }
+
             mod.semantic();
             if (mod.needmoduleinfo)
             {

src/dmodule.d

@@ -114,12 +114,15 @@ extern (C++) class Package : ScopeDsymbol
 {
 public:
     PKG isPkgMod;
+    uint tag;        // auto incremented tag, used to mask package tree in scopes
     Module mod;     // !=null if isPkgMod == PKGmodule
 
     final extern (D) this(Identifier ident)
     {
         super(ident);
         this.isPkgMod = PKGunknown;
+        __gshared uint packageTag;
+        this.tag = packageTag++;
     }
 
     override const(char)* kind() const
@@ -859,6 +862,7 @@ public:
             p.parent = this.parent;
             p.isPkgMod = PKGmodule;
             p.mod = this;
+            p.tag = this.tag; // reuse the same package tag
             p.symtab = new DsymbolTable();
             s = p;
         }
@@ -889,6 +893,7 @@ public:
                      */
                     pkg.isPkgMod = PKGmodule;
                     pkg.mod = this;
+                    pkg.tag = this.tag; // reuse the same package tag
                 }
                 else
                     error(md ? md.loc : loc, "from file %s conflicts with package name %s", srcname, pkg.toChars());

src/dsymbol.d

@@ -1211,6 +1211,9 @@ private:
     Dsymbols* importedScopes;
     PROTKIND* prots;            // array of PROTKIND, one for each import
 
+    import ddmd.root.array : BitArray;
+    BitArray accessiblePackages;// whitelist of accessible (imported) packages
+
 public:
     final extern (D) this()
     {
@@ -1433,6 +1436,19 @@ public:
         }
     }
 
+    final void addAccessiblePackage(Package p)
+    {
+        if (accessiblePackages.length <= p.tag)
+            accessiblePackages.length = p.tag + 1;
+        accessiblePackages[p.tag] = true;
+    }
+
+    final bool isPackageAccessible(Package p)
+    {
+        return p.tag < accessiblePackages.length &&
+            accessiblePackages[p.tag];
+    }
+
     override final bool isforwardRef()
     {
         return (members is null);

src/dsymbol.h

@@ -288,6 +288,8 @@ class ScopeDsymbol : public Dsymbol
     Dsymbols *importedScopes;   // imported Dsymbol's
     PROTKIND *prots;            // array of PROTKIND, one for each import
 
+    BitArray accessiblePackages;
+
 public:
     ScopeDsymbol();
     ScopeDsymbol(Identifier *id);

src/expression.d

@@ -8304,6 +8304,8 @@ public:
                  */
                 if (Declaration d = s.isDeclaration())
                     checkAccess(loc, sc, null, d);
+                if (auto p = s.isPackage())
+                    checkAccess(loc, sc, p);
 
                 // if 's' is a tuple variable, the tuple is returned.
                 s = s.toAlias();

src/module.h

@@ -37,6 +37,7 @@ class Package : public ScopeDsymbol
 {
 public:
     PKG isPkgMod;
+    unsigned tag;       // auto incremented tag, used to mask package tree in scopes
     Module *mod;        // != NULL if isPkgMod == PKGmodule
 
     Package(Identifier *ident);

src/root/array.d

@@ -204,4 +204,54 @@ public:
     {
         return data[0 .. dim];
     }
+
+    extern (D) inout(T)[] opSlice(size_t a, size_t b) inout
+    {
+        assert(a <= b && b <= dim);
+        return data[a .. b];
+    }
+}
+
+struct BitArray
+{
+    size_t length() const
+    {
+        return len;
+    }
+
+    void length(size_t nlen)
+    {
+        ptr = cast(size_t*)mem.xrealloc(ptr, (nlen + 7) / 8);
+        len = nlen;
+    }
+
+    bool opIndex(size_t idx) const
+    {
+        import core.bitop : bt;
+
+        assert(idx < length);
+        return !!bt(ptr, idx);
+    }
+
+    void opIndexAssign(bool val, size_t idx)
+    {
+        import core.bitop : btc, bts;
+
+        assert(idx < length);
+        if (val)
+            bts(ptr, idx);
+        else
+            btc(ptr, idx);
+    }
+
+    @disable this(this);
+
+    ~this()
+    {
+        mem.xfree(ptr);
+    }
+
+private:
+    size_t len;
+    size_t *ptr;
 }

src/root/array.h

@@ -237,4 +237,23 @@ struct Array
     }
 };
 
+struct BitArray
+{
+    BitArray()
+      : len(0)
+      , ptr(NULL)
+    {}
+
+    ~BitArray()
+    {
+        mem.xfree(ptr);
+    }
+
+    size_t len;
+    size_t *ptr;
+
+private:
+    BitArray(const BitArray&);
+};
+
 #endif

test/compilable/imports/a313.d

@@ -0,0 +1,8 @@
+module imports.a313;
+
+// adds private package imports
+private import imports.b313;
+// adds private package core
+private import core.stdc.stdio;
+// adds public alias cstdio
+public alias cstdio = core.stdc.stdio;

test/compilable/imports/b313.d

@@ -0,0 +1,7 @@
+module imports.b313;
+
+void bug()
+{
+    // scope has access to it's own module
+    imports.b313.bug();
+}

test/compilable/imports/f313.d

@@ -0,0 +1,6 @@
+ // different module declaration not used for access check
+module foo.bar;
+
+void bug()
+{
+}

test/compilable/imports/pkg313/c313.d

@@ -0,0 +1,5 @@
+module imports.pkg313.c313;
+
+void bug()
+{
+}

test/compilable/imports/pkgmod313/mod.d

@@ -0,0 +1,3 @@
+module imports.pkgmod313.mod;
+
+void bar() {}

test/compilable/imports/pkgmod313/package.d

@@ -0,0 +1,5 @@
+module imports.pkgmod313;
+
+public import imports.pkgmod313.mod;
+
+void foo() {}

test/compilable/test313a.d

@@ -0,0 +1,26 @@
+/*
+REQUIRED_ARGS: -de
+*/
+module test313;
+
+import imports.a313;
+
+void test1()
+{
+    import imports.b313;
+    imports.b313.bug();
+}
+
+void test2()
+{
+    cstdio.printf("");
+}
+
+import imports.pkg313.c313;
+void test3()
+{
+    imports.pkg313.c313.bug();
+}
+
+// private symbols from other modules are still visible
+static assert(core.stringof == "package core");

test/compilable/test313b.d

@@ -0,0 +1,6 @@
+// REQUIRED_ARGS: -de
+void test1()
+{
+    import core.stdc.stdio;
+    core.stdc.stdio.printf("");
+}

test/compilable/test313c.d

@@ -0,0 +1,8 @@
+// REQUIRED_ARGS: -de
+import imports.pkgmod313;
+
+void test()
+{
+    imports.pkgmod313.foo();
+    imports.pkgmod313.bar();
+}

test/compilable/test313d.d

@@ -0,0 +1,9 @@
+// first imported as package
+// EXTRA_SOURCES: imports/pkgmod313/mod.d
+// REQUIRED_ARGS: -de
+import imports.pkgmod313; // then as package module
+
+void test()
+{
+    imports.pkgmod313.foo();
+}

test/compilable/test313e.d

@@ -0,0 +1,9 @@
+// first resolved as package, then created as module (with name package)
+// EXTRA_SOURCES: imports/pkgmod313/mod.d imports/pkgmod313/package.d
+// REQUIRED_ARGS: -de
+import imports.pkgmod313; // then imported as package module
+
+void test()
+{
+    imports.pkgmod313.foo();
+}