Bug 65311 – [EFL] Replace strerror with strerror_r.

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit EFL

[KwangHyuk]

As strerror_r is a thread safty api corresponding to strerror, strerror_r is more desirable for the error logging.

[KwangHyuk]

Created attachment 102246 [details]
Patch

[WebKit Review Bot]

Attachment 102246 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[KwangHyuk]

Created attachment 102247 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Did you actually experience some problem caused by the use of strerror instead of strerror_r?

[KwangHyuk]

> Did you actually experience some problem caused by the use of strerror instead of strerror_r?

No, in fact, it was just reported by our static analysis tool for the secure coding.
But, I think webkit efl should be thread safe and secure as much as possible.

[Raphael Kubo da Costa (:rakuco)]

Personally, I don't see this as being really needed, it looks like a change for the change's sake: strerror_r would be useful if you actually used the string it stores in buf; if you use it the same way you use strerror there shouldn't be any difference.

For this use case here, I think there is no need for this patch.

[Lucas De Marchi]

Comment on attachment 102247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102247&action=review

> Source/WebKit/efl/ewk/ewk_tiled_private.h:35
> +#define OOM(op, size) \
> +    char buf[256]; \
> +    CRITICAL("could not %s %zd bytes: %s", op, size, strerror_r(errno, buf, sizeof(buf)))
> +

Besides what Raphael said, this is wrong. See this piece of code:

int function(void)
{
    char *buf;

    buf = malloc(200);
    if (!buf)
        OOM("allocate", 200);
}


Bang... you get a compiler error: `error: expected expression before ‘char’'

[Lucas De Marchi]

(In reply to comment #7)
> (From update of attachment 102247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102247&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_private.h:35
> > +#define OOM(op, size) \
> > +    char buf[256]; \
> > +    CRITICAL("could not %s %zd bytes: %s", op, size, strerror_r(errno, buf, sizeof(buf)))
> > +


A patch worth doing would be to change this message to use "%m" instead of strerror() -- and removing "#include <errno.h>" where possible.

[Lucas De Marchi]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 102247 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=102247&action=review
> > 
> > > Source/WebKit/efl/ewk/ewk_tiled_private.h:35
> > > +#define OOM(op, size) \
> > > +    char buf[256]; \
> > > +    CRITICAL("could not %s %zd bytes: %s", op, size, strerror_r(errno, buf, sizeof(buf)))
> > > +
> 
> 
> A patch worth doing would be to change this message to use "%m" instead of strerror() -- and removing "#include <errno.h>" where possible.

Ahnn... this is true if we don't mind to support a libc other than glibc. Otherwise it should remain as is.

[KwangHyuk]

> Ahnn... this is true if we don't mind to support a libc other than glibc. 
> Otherwise it should remain as is.

Thank you for your detail idea,

"%m" looks good to me too because glic's printf seems to use strerror_r.
And printf will ignore "%m" if it will not available.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> > Ahnn... this is true if we don't mind to support a libc other than glibc. 
> > Otherwise it should remain as is.
> 
> Thank you for your detail idea,
> 
> "%m" looks good to me too because glic's printf seems to use strerror_r.
> And printf will ignore "%m" if it will not available.

No, %m is not portable and should not be used. I don't think we only have glibc in mind.

IMO, the bug should be closed.

[KwangHyuk]

> IMO, the bug should be closed.

Ok.
Then, I better close this.

[KwangHyuk]

I agree to close this bug.