505731 - Wrong shell32 linking arguments on MinGW.

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Jacek Caban]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

shell32 linking option is hardcoded to shell32.lib, but on MinGW it should be -lshell32.

Reproducible: Always

Comment 1

[Jacek Caban]

Attachment: Fix (checked in)

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

@Glen, Are the freebl Makefiles part of the FIPS-evaluated code?

@me: we link shell32 for win_rand's call to SHGetSpecialFolderPathW

Comment 3

[Wan-Teh Chang]

Comment on attachment 389934 [details] [diff] [review]
Fix (checked in)

r=wtc.

Comment 4

[cls]

Jack,  what version of mingw gcc & w32api are you using?  In order to get freebl to actually see the SHGetSpecialFolderPathW symbol at link time, I had to build NSS with -D_WIN32_IE=0x0500. I see this in both the CVS & hg comm-central trees. I'm using the mingw32 rpms from Fedora 11 (gcc 4.4.0, w32api 3.13, runtime 3.15).

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/private/primpl.h&rev=3.91&mark=65-67#57

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Hmm.  I thought we were doing this in NSS too.  
If NSS depends on WINVER being 0x500 but doesn't set it, that's a bug.

Comment 7

[Jacek Caban]

(In reply to comment #4)
> Jack,  what version of mingw gcc & w32api are you using?

I'm using gcc-4.3.6 (newer versions suffer from bug 445294), w32api 3.13 and runtime 3.14. What see is a bug in MinGW. I've filled a bug, but they don't seem interested:
http://sourceforge.net/tracker/?func=detail&aid=2824763&group_id=2435&atid=102435
(I can't send them a patch myself because I work on Wine and they don't agree with our method of writing header files). There are more MinGW bugs. I had to fix a few headers and use Wine headers for missing ones as well as use some Wine importlibs. See:
http://wiki.winehq.org/BuildingWineGecko
Also a few patches to Mozilla are needed to compile, all of them are in bugzilla, search for bugs reported by me.

Comment 8

[glen beasley]

in response to comment #2 yes the freebl Makefiles are part of the FIPS evaluated code. 
Unfortunately any code in 
mozilla/security/nss/lib/freebl 
mozilla/security/nss/lib/softoken
is considered FIPS. 
If a change is approved for check-in then it should be checked into 
SOFTOKEN_3_13_BRANCH.

Comment 9

[Jacek Caban]

I'm not sure I understand. Can this patch be checked-in? Is there something I should do to allow checking it in (I don't have rights to do it myself anyways)?

Comment 10

[Jacek Caban]

wtc, could you please land this patch?

Comment 12

[Jacek Caban]

Attachment: Extended fix

There is another problem with mingw compilation. w32api headers contain _WIN32_IE >= 0x0400 guard around SHGetSpecialFolderPath. It's an incompatibility with MS SDK, so it should be fixed in w32api. I've tried to get it fixed in w32api, but they have strange rules, and they won't accept the fix because that's not what MSDN says it should be.

We may fix it in NSS by simply defining _WIN32_IE. It can't hurt, so I propose to extend previous patch.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389934 [details] [diff] [review]
Fix (checked in)

new patch obsoletes this old one.

Comment 14

[Jacek Caban]

Attachment: fix with _WIN32_IE set to 501

I've raised _WIN32_IE to 501 as that's what mingw-w64 shlobj.h version expects. wtc, please let me know if I should ask someone else for review.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 448717 [details] [diff] [review]
fix with _WIN32_IE set to 501

We cannot change the Windows version compatibility requirements for all Windows builds simply at will.  

If MinGW headers need a special version number, then the setting of that version number should be in some ifdef for MinGW.

Comment 16

[Wan-Teh Chang]

Comment on attachment 389934 [details] [diff] [review]
Fix (checked in)

I checked in the patch on the SOFTOKEN_3_13_BRANCH (NSS 3.13).

Checking in config.mk;
/cvsroot/mozilla/security/nss/lib/freebl/config.mk,v  <--  config.mk
new revision: 1.23.4.1; previous revision: 1.23
done

I checked in the patch on the NSS trunk (NSS 3.12.7).

Checking in config.mk;
/cvsroot/mozilla/security/nss/lib/freebl/config.mk,v  <--  config.mk
new revision: 1.24; previous revision: 1.23
done

Comment 17

[Wan-Teh Chang]

Comment on attachment 448717 [details] [diff] [review]
fix with _WIN32_IE set to 501

According to http://msdn.microsoft.com/en-us/library/aa383745(VS.85).aspx,
a _WIN32_IE value of 0x0501 means Internet Explorer 5.01.
http://support.microsoft.com/kb/267954 shows Internet Explorer 5.01
supports Windows 2000.  So it should be fine to define _WIN32_IE as
0x0501.  It would be nice to add a comment though.

This Wikipedia article shows IE 6 supports Windows 2000, too:
http://en.wikipedia.org/wiki/Internet_Explorer_6
IE 6 is considered insecure today, so it should be fine for NSS to require
IE 5.01.

Comment 18

[Jacek Caban]

wtc, thanks for checkin.

wtc, nelson: Adding _WIN32_IE doesn't change requirements at all. MS added _WIN32_IE-based guards to their headers so that one won't use newer API than explicitly requested. The problem is that it's not used consequently. SHGetSpecialFolderPath is a good example. Although MSDN says that IE4 is required, it's not protected by _WIN32_IE. It means that we have to be careful about used function no matter what _WIN32_IE is set to.

To complicate the story, w32api developers believe MSDN. They've added a guard and didn't want to remove it even when I gave them a test showing that it's wrong. Now there is new set of headers for win64 called mingw-w64. They guarded the whole file by checking _WIN32_IE > 500. It's all an ugly mess now (that's why Wine version doesn't contain any such guards)...

Getting that, setting _WIN32_IE without MINGW guard seems like the best solution - won't hurt MSC and avoids useless #ifdefs, but I will attach a patch as you requested.

Comment 19

[Jacek Caban]

Attachment: Define _WIN32_IE to 501 on mingw.

A patch without mingw guard.

Comment 20

[Jacek Caban]

Attachment: Define _WIN32_IE to 501 on mingw.

A patch with mingw guard. Please select preferred.

Comment 21

[Wan-Teh Chang]

Comment on attachment 448996 [details] [diff] [review]
 Define _WIN32_IE to 501 on mingw.

Jacek:

I suggest that you add -D_WIN32_IE=0x0501 to
security/nss/lib/freebl/config.mk, inside the
ifdef NS_USE_GCC that you just added:

> ifndef WINCE
>+ifdef NS_USE_GCC
>+OS_LIBS += -lshell32
>+else
> OS_LIBS += shell32.lib
> endif
>+endif

You can do
  DEFINES += -D_WIN32_IE=0x0501

Comment 22

[Jacek Caban]

Attachment: Define _WIN32_IE to 0x0501 on mingw - wtc's solution

This way we won't limit _WIN32_IE setting to required minimum, but it may be better to avoid similar problems in future. Thanks.

Comment 23

[Jacek Caban]

It's no longer needed in recent mingw-w64