558415 - Handle libc functions not implemented for Android

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Alex Pakhotin (:alexp)]

Some rarely used base functionality in not implemented in Bionic C library.
For example, the following functions: getprotobyname(), getprotobynumber(), shared memory.
Some workaround has to be implemented, or at least asserts to be added, to make sure these functions are not called by the applications using NSPR.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

blocking-fennec 2.0+ for making sure we don't hit this when running Fennec

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Still blocking?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

alex, can you put a patch together to assert where appropriate?

Comment 4

[Alex Pakhotin (:alexp)]

Actually shared memory functions are already disabled for Android with reporting an error using PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0):
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/memory/prshm.c#59

The test just does not show this error.

Do we want to use the same approach with error code setting for getprotobyname/getprotobynumber as well or add PR_ASSERT for all of these functions?

Comment 5

[Alex Pakhotin (:alexp)]

Attachment: Report PR_NOT_IMPLEMENTED_ERROR for getprotobyname/getprotobynumber

Comment 6

[Wan-Teh Chang]

Comment on attachment 481126 [details] [diff] [review]
Report PR_NOT_IMPLEMENTED_ERROR for getprotobyname/getprotobynumber

Since these are stubs for libc functions (rather than NSPR functions), you should set errno instead of calling PR_SetError.

> /* Android's Bionic libc system includes prototypes for these in netdb.h,
>  * but doesn't actually include implementations.  It uses the 5-arg form,
>  * so these functions end up not matching the prototype.  So just rename
>  * them if not found.
>  */

This comment says these functions have 5 arguments, but your stubs have
only one argument.  Why?

Comment 7

[Alex Pakhotin (:alexp)]

(In reply to comment #6)
> This comment says these functions have 5 arguments, but your stubs have
> only one argument.  Why?

As the comment says, the Android libc does not have implementation for "*_r" versions of the functions, so we used our own versions of getprotobyname_r/getprotobynumber_r functions with one argument, which just called getprotobyname/getprotobynumber. But as the prototypes of the standard functions are in the headers, we had to rename our functions with #define-s.

My change handles another problem: even though getprotobyname/getprotobynumber functions do actually exist in Android libc, they just output "FIX ME! implement getprotoXXX()", so it doesn't make sense to call them right now.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 481126 [details] [diff] [review]
Report PR_NOT_IMPLEMENTED_ERROR for getprotobyname/getprotobynumber

I think the comment should be clearer. Particularly it should note why we're overriding bionic's broken impl.

Comment 9

[Alex Pakhotin (:alexp)]

Attachment: Report PR_NOT_IMPLEMENTED_ERROR for getprotobyname/getprotobynumber (v2)

Added a comment.
f+ blassey

Comment 10

[Wan-Teh Chang]

(In reply to comment #7)
>
> My change handles another problem: even though getprotobyname/getprotobynumber
> functions do actually exist in Android libc, they just output "FIX ME!
> implement getprotoXXX()", so it doesn't make sense to call them right now.

If the getprotobyname/getprotobynumber functions return NULL without crashing, it is still better to call them, so that we won't have to change this code when getprotobyname/getprotobynumber are implemented in the future.  Are you bothered by the "FIX ME! implement getprotoXXX()" messages they write to the log?

Comment 11

[Wan-Teh Chang]

Attachment: Report PR_NOT_IMPLEMENTED_ERROR for getprotobyname/getprotobynumber (v3), by Alex Pakhotin

This is the same as patch v2 (attachment 487180 [details] [diff] [review]) except that I reformatted the comment.

You can check this in after alexp answers my question in comment 10.

Author: Alex Pakhotin <[e-mail]>
Bug 558415 - Handle libc getprotobyname/getprotobynumber functions not implemented for Android.

Comment 12

[Alex Pakhotin (:alexp)]

(In reply to comment #10)
> If the getprotobyname/getprotobynumber functions return NULL without crashing,
> it is still better to call them, so that we won't have to change this code when
> getprotobyname/getprotobynumber are implemented in the future.  Are you
> bothered by the "FIX ME! implement getprotoXXX()" messages they write to the
> log?

We are just worried about our code, which may depend on those functions. Getting PR_NOT_IMPLEMENTED_ERROR will give an idea why something may not work, if these functions will be used. Eyeballing the system log might not always be possible.

Comment 13

[Wan-Teh Chang]

alexp: I found the answer to my question in the source code.
getprotobyname and getprotobynumber are defined as follows
in bionic/libc/bionic/stubs.c:

struct protoent *getprotobyname(const char *name)
{
    fprintf(stderr, "FIX ME! implement %s() %s:%d\n", __FUNCTION__, __FILE__, __LINE__);
    return NULL;
}

struct protoent *getprotobynumber(int proto)
{
    fprintf(stderr, "FIX ME! implement %s() %s:%d\n", __FUNCTION__, __FILE__, __LINE__);
    return NULL;
}

So I see no advantage of your patch, other than
not writing error messages to stderr.

Note that the PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0) calls
you added are ignored by NSPR because getprotobyname_r and
getprotobynumber_r are not NSPR functions.  The man page does
not specify how these functions report errors:
http://www.opengroup.org/onlinepubs/009695399/functions/endprotoent.html

Also, NSPR sets a different error code here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prnetdb.c&rev=3.61&mark=1273-1274,1277#1269

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prnetdb.c&rev=3.61&mark=1357-1358,1361#1353

In summary, my recommendation is to mark this bug WONTFIX.
If you insist, we probably should change
    PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0);
to something like
    errno = ENOSYS;  /* Function not implemented */

Comment 14

[Wan-Teh Chang]

Attachment: Report errno=ENOSYS for getprotobyname/getprotobynumber (v4), by Alex Pakhotin

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

I don't think patch adds value. We should figure out if we're depending on these functions though and make sure that we have appropriate fallbacks in place.