hyc@OpenLDAP.org writes:
Modified Files: io.c 1.120 -> 1.121 ITS#5580 fix length decoding, verified with PROTOS
Keep the sock_errset() if returning LBER_DEFAULT and sblen < 0?
And maybe the variable used like i in the changed code must be ber_slen_t instead of int, I'm a bit too tired to check now. The remaining use of i as int looks OK.
Hallvard B Furuseth wrote:
hyc@OpenLDAP.org writes:
Modified Files: io.c 1.120 -> 1.121 ITS#5580 fix length decoding, verified with PROTOS
Keep the sock_errset() if returning LBER_DEFAULT and sblen< 0?
No. ber_int_sb_read() will cause errno to already be set if it cannot fulfill the request.
And maybe the variable used like i in the changed code must be ber_slen_t instead of int
Why?
Howard Chu writes:
Keep the sock_errset() if returning LBER_DEFAULT and sblen< 0?
No. ber_int_sb_read() will cause errno to already be set if it cannot fulfill the request.
Sorry, I meant sblen >= 0 of course. That can be a read() result which does not set errno.
And maybe the variable used like i in the changed code must be ber_slen_t instead of int
Why?
Same type as sblen, used for the full ber buffer size. Haven't looked closely enough to see if the particular pointer diff can only be a few bytes or not.
Hallvard B Furuseth wrote:
Howard Chu writes:
Keep the sock_errset() if returning LBER_DEFAULT and sblen< 0?
No. ber_int_sb_read() will cause errno to already be set if it cannot fulfill the request.
Sorry, I meant sblen>= 0 of course. That can be a read() result which does not set errno.
No, doesn't matter. A blocking read that returns 0 means of course that the connection was closed. A non-blocking read that returns 0 will set errno.
Hm. If sblen >0 and <i then we will lose bytes, need to fix that.
And maybe the variable used like i in the changed code must be ber_slen_t instead of int
Why?
Same type as sblen, used for the full ber buffer size. Haven't looked closely enough to see if the particular pointer diff can only be a few bytes or not.
In this section of code, the difference can only be [0-4] bytes.
Looking closer now that I'm awake...
Howard Chu writes:
Hallvard B Furuseth wrote:
Howard Chu writes:
Keep the sock_errset() if returning LBER_DEFAULT and sblen< 0?
No. ber_int_sb_read() will cause errno to already be set if it cannot fulfill the request.
Sorry, I meant sblen>= 0 of course. That can be a read() result which does not set errno.
No, doesn't matter. A blocking read that returns 0 means of course that the connection was closed.
But the caller doesn't know that. What it knows is that ber_get_next() returned LBER_DEFAULT. Anyway, that part of the code seems to be gone now.
A non-blocking read that returns 0 will set errno.
Not on my host (Linux). And not in the Posix spec that I can see.
In any case, it looks like the error number handling is incomplete:
ber_get_next() can do sock_errset(0) before calling ber_int_sb_read() and returning LBER_DEFAULT if zero result, however that function can set errno spuriously and then return zero. At least in an EINTR loop there or in sb_rdahead_read(). I don't know if the read OS call itself is one of the functions which can set errno (or WSASetLastError) spuriously on some OSes.
ber_get_next() can return LBER_DEFAULT due to memory allocation error, I don't suppose that does sock_errset(). Haven't checked if all variants set errno either before failing.
On the caller side, slapd/connection.c treats all other return values than LDAP_TAG_MESSAGE as LBER_DEFAULT and examines sock_errno(). libldap/result.c does Debug() before reading sock_errno(). Debug() can change errno.
Hallvard B Furuseth wrote:
Looking closer now that I'm awake...
Howard Chu writes:
Hallvard B Furuseth wrote:
Howard Chu writes:
Keep the sock_errset() if returning LBER_DEFAULT and sblen< 0?
No. ber_int_sb_read() will cause errno to already be set if it cannot fulfill the request.
Sorry, I meant sblen>= 0 of course. That can be a read() result which does not set errno.
No, doesn't matter. A blocking read that returns 0 means of course that the connection was closed.
But the caller doesn't know that. What it knows is that ber_get_next() returned LBER_DEFAULT.
And that's all that matters. If the caller doesn't see one of the try-again error codes, then the connection is dropped. In the case of read returning 0, that means the connection was closed by the remote side anyway.
Anyway, that part of the code seems to be gone now.
A non-blocking read that returns 0 will set errno.
Not on my host (Linux). And not in the Posix spec that I can see.
My mistake; actually a non-blocking read will return -1 (with errno set) when there's no data available. A return of 0 always means the connection was closed.
In any case, it looks like the error number handling is incomplete:
ber_get_next() can do sock_errset(0) before calling ber_int_sb_read() and returning LBER_DEFAULT if zero result, however that function can set errno spuriously and then return zero. At least in an EINTR loop there or in sb_rdahead_read(). I don't know if the read OS call itself is one of the functions which can set errno (or WSASetLastError) spuriously on some OSes.
Irrelevant.
ber_get_next() can return LBER_DEFAULT due to memory allocation error, I don't suppose that does sock_errset(). Haven't checked if all variants set errno either before failing.
Irrelevant. Anything besides EAGAIN/EWOULDBLOCK (including 0) is a fatal error and the connection is dropped.
On the caller side, slapd/connection.c treats all other return values than LDAP_TAG_MESSAGE as LBER_DEFAULT and examines sock_errno().
Yes.
libldap/result.c does Debug() before reading sock_errno(). Debug() can change errno.
That should probably be changed.
Howard Chu writes:
But the caller doesn't know that. What it knows is that ber_get_next() returned LBER_DEFAULT.
And that's all that matters.
As long as the try-again error code isn't spurious, which is what I don't know when/how can happen. Though hopefully the caller will get another error when it tries again.
It does mean the caller can log the wrong error code though. In particular after a non-error return.
If the caller doesn't see one of the try-again error codes, then the connection is dropped.
Hallvard B Furuseth wrote:
Howard Chu writes:
But the caller doesn't know that. What it knows is that ber_get_next() returned LBER_DEFAULT.
And that's all that matters.
As long as the try-again error code isn't spurious, which is what I don't know when/how can happen. Though hopefully the caller will get another error when it tries again.
It does mean the caller can log the wrong error code though. In particular after a non-error return.
What are you talking about, re: spurious error codes? System calls only set errno when they fail. System calls return -1 when they fail. sb_rdahead_read() doesn't set errno either... That's why we have to zero errno out before doing the read, to make sure a stale error code from some previous op isn't hanging around - because read() *won't* touch it when it returns success.
If the caller doesn't see one of the try-again error codes, then the connection is dropped.
Howard Chu writes:
What are you talking about, re: spurious error codes? System calls only set errno when they fail.
No. From POSIX, IEEE Std 1003.1 of 2004, susv3/functions/errno.html: "The value of errno should only be examined when it is indicated to be valid by a function's return value. (...) The setting of errno after a successful call to a function is unspecified unless the description of that function specifies that errno shall not be modified." As for non-POSIX, I have no idea.
The C standard is a bit gentler for its own functions. From C99 7.5p3: "The value of errno may be set to nonzero by a library function call whether or not there is an error, provided the use of errno is not documented in the description of the function in this International Standard." Thus e.g. strtol() cannot set a spurious errno because its description says when to set errno = ERANGE.
I wrote:
No. From POSIX, IEEE Std 1003.1 of 2004, susv3/functions/errno.html: "The value of errno should only be examined when it is indicated to be valid by a function's return value. (...) The setting of errno after a successful call to a function is unspecified unless the description of that function specifies that errno shall not be modified."
BTW, the change history says this is new in the 2004 edition. I'm guessing that was done to reflect existing practice. Pre-POSIX errno was an utter mess, but the growing conformance to various standards like POSIX has at least imporved and clarified the situation.
Though the same page also says "An application that needs to examine the value of errno to determine the error should set it to 0 before a function call, then inspect it before a subsequent function call." Not quite sure what that means. Maybe a holdover from an earlier version? Certainly it's safer for functions that are not required to set errno, or when writing for OSes using different standards (or not yet properly supporting any standard but their own).
-
Hallvard B Furuseth -
Howard Chu