Closed Bug 828730 Opened 8 years ago Closed 8 years ago

Cannot generate test apps for test_signed_apps.js on Linux (crash)

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: briansmith, Assigned: amac)

Details

Attachments

(1 file, 2 obsolete files)

The generate.sh script works correctly on Windows 7/8 with python 2.7.2 when using the NSS in the Gecko source tree (built using the NSS build system). However, it fails when executed on Linux.
I didn't investigate much yet, but I suspect this might be a 32-bit/64-bit issue. My Ubuntu test was done using 64-bit Ubuntu by my Windows tests were done using 32-bit NSS on 32-bit python. But, I tried 32-bit and 64-bit NSS (build NSS with USE_64=1 to get a 64-bit build) and both still crashed. Perhaps the ctypes types I am using are not right.
I found the problem. I don't know how (or why) it's working on Windows, cause it's most definitely broken.

The problem is on the password callback. The password callback is returning a Python string:

  def callback(slot, retry, arg):
    return password

which later NSS tries to free (in [1]). And since it cannot free it it breaks.

I've done a ugly workaround just copying the string in C (via strdup) only for posix machines. On the WIP attached the libc path is hardcoded, that should be fixed before pushing this, I guess. In any case, it fixes the problem. 

[1] https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11auth.c#621
Attachment #700180 - Flags: feedback?(bsmith)
Comment on attachment 700180 [details] [diff] [review]
WIP, fixes the breakage but not very pretty

Review of attachment 700180 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/test_signed_apps/nss_ctypes.py
@@ +69,5 @@
>  nss.PK11_SetPasswordFunc.restype = None
>  def SetPasswordContext(password):
>    def callback(slot, retry, arg):
> +    if use_libc: 
> +      return libc.strdup(password)

OK, makes sense. I think we should just implement our own strdup in terms of ctypes.create_string_buffer and ctypes.memmove, instead of looking up strdup in libc, so that it works cross-platform. If you do that then I can r+ it.
Attachment #700180 - Flags: feedback?(bsmith) → feedback+
(In reply to Brian Smith (:bsmith) from comment #3)
> Comment on attachment 700180 [details] [diff] [review]
> WIP, fixes the breakage but not very pretty
> 
> Review of attachment 700180 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/tests/unit/test_signed_apps/nss_ctypes.py
> @@ +69,5 @@
> >  nss.PK11_SetPasswordFunc.restype = None
> >  def SetPasswordContext(password):
> >    def callback(slot, retry, arg):
> > +    if use_libc: 
> > +      return libc.strdup(password)
> 
> OK, makes sense. I think we should just implement our own strdup in terms of
> ctypes.create_string_buffer and ctypes.memmove, instead of looking up strdup
> in libc, so that it works cross-platform. If you do that then I can r+ it.

I did try using create_string_buffer yesterday night before reverting to the good old strdup, but turns out that mutable doesn't mean free-able :). It fails with the same error, on other words.

I'll see if there's any strdup-like function on NSPR that I can use. Other than that... there's actually a libc-like library for Windows but I think it's part of the MSVC redistributable.
(In reply to Antonio Manuel Amaya Calvo from comment #4)
> I'll see if there's any strdup-like function on NSPR that I can use. Other
> than that... there's actually a libc-like library for Windows but I think
> it's part of the MSVC redistributable.

NSPR has PL_strdup that we should be able to use.
Ok, I checked and there was a strdup like function in NSPR. So I just changed yesterday hack to use the NSPR function instead (which is more correct in any case). Since NSPR is a requirement, this doesn't introduce any new dependencies and should work on all platforms.
Assignee: bsmith → amac
Attachment #700180 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #700328 - Flags: review?(bsmith)
Same as 700328, with fixed spaces (maybe save a review round this way ;) ).
Attachment #700328 - Attachment is obsolete: true
Attachment #700328 - Flags: review?(bsmith)
Attachment #700331 - Flags: review?(bsmith)
Comment on attachment 700331 [details] [diff] [review]
Fix using PL_strdup to reserve memory for the password

Review of attachment 700331 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1d90aba921
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3aca4d524d15

Thanks! I tested this on Windows.

I remember now that Windows Python did warn about a problem here, but the problem it warned about was a memory leak, and I thought it was benign. Shame on me.
Attachment #700331 - Flags: review?(bsmith) → review+
Nominating to get it as tef+, this is needed for our certification.
blocking-basecamp: --- → ?
blocking-b2g: tef? → tef+
blocking-basecamp: ? → -
https://hg.mozilla.org/mozilla-central/rev/9f1d90aba921
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
You need to log in before you can comment on or make changes to this bug.