[vlc-commits] [Git][videolan/vlc][master] 6 commits: access: cache: don't call free_cb from entry_Delete
Hugo Beauzée-Luyssen (@chouquette)
gitlab at videolan.org
Tue Mar 22 15:38:24 UTC 2022
Hugo Beauzée-Luyssen pushed to branch master at VideoLAN / VLC
Commits:
dc4b8c6b by Thomas Guillem at 2022-03-22T15:16:00+00:00
access: cache: don't call free_cb from entry_Delete
No functional changes, vlc_access_cache_entry_Delete() is not (yet)
called from the outside.
- - - - -
29b85334 by Thomas Guillem at 2022-03-22T15:16:00+00:00
smb2: don't use sys->error_status while opening
But return it directly from function and sub functions.
- - - - -
c2370913 by Thomas Guillem at 2022-03-22T15:16:00+00:00
smb2: rework error handling
- Always call VLC_SMB2_SET_ERROR() in case of error,
- Don't loose the returned error code
- Fallback to -EINVAL in case of unknown error (very unlikely case)
- - - - -
924c9515 by Thomas Guillem at 2022-03-22T15:16:00+00:00
smb2: destroy the context in case of error
This fixes a potential stack-buffer-overflow when destroying a context
from Close() if an operation was aborted. Indeed, the smb2_destroy()
function might trigger callbacks with private data that was allocated on
an old function stack. To fix this issue, always destroy the smb2
context immediately after an error (when the struct vlc_smb2_op is
valid).
This issue is currently hidden by the teardown mechanism (but still
possible), that always try to close gracefully in case of error.
- - - - -
cf7d48cd by Thomas Guillem at 2022-03-22T15:16:00+00:00
smb2: remove teardown handling
If interrupted by the user, just close the connection whitout sending a
close request and don't save the context in the cache in that case.
- - - - -
ac95bf19 by Thomas Guillem at 2022-03-22T15:16:00+00:00
smb2: always use smb2 timeout
If the smb2_timeout was valid, then not valid (infinite), the last value
was not taken into account.
- - - - -
3 changed files:
- modules/access/cache.c
- modules/access/cache.h
- modules/access/smb2.c
Changes:
=====================================
modules/access/cache.c
=====================================
@@ -38,7 +38,6 @@ vlc_access_cache_entry_Delete(struct vlc_access_cache_entry *entry)
free(entry->url);
free(entry->username);
- entry->free_cb(entry->context);
free(entry);
}
@@ -86,6 +85,7 @@ vlc_access_cache_Thread(void *data)
vlc_mutex_unlock(&cache->lock);
+ entry->free_cb(entry->context);
vlc_access_cache_entry_Delete(entry);
vlc_mutex_lock(&cache->lock);
@@ -133,7 +133,10 @@ vlc_access_cache_Destroy(struct vlc_access_cache *cache)
struct vlc_access_cache_entry *entry;
vlc_list_foreach(entry, &cache->entries, node)
+ {
+ entry->free_cb(entry->context);
vlc_access_cache_entry_Delete(entry);
+ }
}
void
@@ -147,6 +150,7 @@ vlc_access_cache_AddEntry(struct vlc_access_cache *cache,
if (!cache->running)
{
vlc_mutex_unlock(&cache->lock);
+ entry->free_cb(entry->context);
vlc_access_cache_entry_Delete(entry);
return;
}
=====================================
modules/access/cache.h
=====================================
@@ -85,6 +85,7 @@ vlc_access_cache_entry_NewSmb(void *context, const char *server,
return entry;
}
+/* Delete the cache entry without firing the free_cb */
void
vlc_access_cache_entry_Delete(struct vlc_access_cache_entry *entry);
=====================================
modules/access/smb2.c
=====================================
@@ -115,8 +115,6 @@ struct access_sys
bool eof;
bool smb2_connected;
- int error_status;
-
struct vlc_access_cache_entry *cache_entry;
};
@@ -125,6 +123,7 @@ struct vlc_smb2_op
struct vlc_logger *log;
struct smb2_context *smb2;
+ struct smb2_context **smb2p;
int error_status;
@@ -138,18 +137,20 @@ struct vlc_smb2_op
} res;
};
-#define VLC_SMB2_OP(access, smb2_) { \
+#define VLC_SMB2_OP(access, smb2p_) { \
.log = access ? vlc_object_logger(access) : NULL, \
- .smb2 = smb2_, \
- .error_status = access ? ((struct access_sys *)access->p_sys)->error_status : 0, \
+ .smb2p = smb2p_, \
+ .smb2 = (assert(*smb2p_ != NULL), *smb2p_), \
+ .error_status = 0, \
.res_done = false, \
};
static inline void
-vlc_smb2_op_reset(struct vlc_smb2_op *op, struct smb2_context *smb2)
+vlc_smb2_op_reset(struct vlc_smb2_op *op, struct smb2_context **smb2p)
{
op->res_done = false;
- op->smb2 = smb2;
+ op->smb2p = smb2p;
+ op->smb2 = *smb2p;
op->error_status = 0;
}
@@ -174,9 +175,16 @@ smb2_check_status(struct vlc_smb2_op *op, int status, const char *psz_func)
static void
smb2_set_error(struct vlc_smb2_op *op, const char *psz_func, int err)
{
- if (op->log)
+ if (op->log && err != -EINTR)
vlc_error(op->log, "%s failed: %d, %s", psz_func, err, smb2_get_error(op->smb2));
- op->error_status = err;
+
+ if (err != 0)
+ op->error_status = err;
+ else if (op->error_status == 0) /* don't override if set via smb2_check_status */
+ op->error_status = -EINVAL;
+
+ smb2_destroy_context(op->smb2);
+ *op->smb2p = NULL;
}
#define VLC_SMB2_CHECK_STATUS(op, status) \
@@ -188,29 +196,8 @@ smb2_set_error(struct vlc_smb2_op *op, const char *psz_func, int err)
#define VLC_SMB2_STATUS_DENIED(x) (x == -ECONNREFUSED || x == -EACCES)
static int
-vlc_smb2_mainloop(struct vlc_smb2_op *op, bool teardown)
+vlc_smb2_mainloop(struct vlc_smb2_op *op)
{
-#define TEARDOWN_TIMEOUT 250 /* in ms */
-
- int timeout = -1;
- int (*poll_func)(struct pollfd *, unsigned, int) = vlc_poll_i11e;
-
- /* vlc_smb2_mainloop() can be called to clean-up after an error, but this
- * function can override the error_status (from async cbs). Therefore,
- * store the original error_status in order to restore it at the end of
- * this call (since we want to keep the first and original error status). */
- int original_error_status = op->error_status;
-
- if (teardown)
- {
- /* Don't use vlc_poll_i11e that will return immediately with the EINTR
- * errno if VLC's input is interrupted. Use the posix poll with a
- * timeout to let a chance for a clean teardown. */
- timeout = TEARDOWN_TIMEOUT;
- poll_func = (void *)poll;
- op->error_status = 0;
- }
-
while (op->error_status == 0 && !op->res_done)
{
int ret, smb2_timeout;
@@ -224,39 +211,17 @@ vlc_smb2_mainloop(struct vlc_smb2_op *op, bool teardown)
p_fds[i].events = events;
p_fds[i].fd = fds[i];
}
- if (smb2_timeout != -1)
- timeout = smb2_timeout;
- if (fds == NULL || (ret = poll_func(p_fds, fd_count, timeout)) < 0)
+ if (fds == NULL || (ret = vlc_poll_i11e(p_fds, fd_count, smb2_timeout)) < 0)
{
- if (errno == EINTR)
- {
- if (op->log)
- vlc_warning(op->log, "vlc_poll_i11e interrupted");
- if (poll_func != (void *) poll)
- {
- /* Try again with a timeout to let the command complete.
- * Indeed, if this command is interrupted, every future
- * commands will fail and we won't be able to teardown. */
- timeout = TEARDOWN_TIMEOUT;
- poll_func = (void *) poll;
- }
- else
- op->error_status = -errno;
- }
- else
- {
- if (op->log)
- vlc_error(op->log, "vlc_poll_i11e failed");
- op->error_status = -errno;
- }
+ if (op->log && errno == EINTR)
+ vlc_warning(op->log, "vlc_poll_i11e interrupted");
+ VLC_SMB2_SET_ERROR(op, "poll", -errno);
}
else if (ret == 0)
{
- if (teardown)
- op->error_status = -ETIMEDOUT;
- else if (smb2_service_fd(op->smb2, -1, 0) < 0)
- VLC_SMB2_SET_ERROR(op, "smb2_service", 1);
+ if (smb2_service_fd(op->smb2, -1, 0) < 0)
+ VLC_SMB2_SET_ERROR(op, "smb2_service", 0);
}
else
{
@@ -264,14 +229,12 @@ vlc_smb2_mainloop(struct vlc_smb2_op *op, bool teardown)
{
if (p_fds[i].revents
&& smb2_service_fd(op->smb2, p_fds[i].fd, p_fds[i].revents) < 0)
- VLC_SMB2_SET_ERROR(op, "smb2_service", 1);
+ VLC_SMB2_SET_ERROR(op, "smb2_service", 0);
}
}
}
int ret = op->error_status == 0 ? 0 : -1;
- if (original_error_status != 0)
- op->error_status = original_error_status;
return ret;
}
@@ -304,7 +267,7 @@ FileRead(stream_t *access, void *buf, size_t len)
{
struct access_sys *sys = access->p_sys;
- if (sys->eof || sys->error_status != 0)
+ if (sys->eof || sys->smb2 == NULL)
return 0;
/* Limit the read size since smb2_read_async() will complete only after
@@ -313,21 +276,19 @@ FileRead(stream_t *access, void *buf, size_t len)
if (len > 262144)
len = 262144;
- struct vlc_smb2_op op = VLC_SMB2_OP(access, sys->smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, &sys->smb2);
op.res.read.len = 0;
- if (smb2_read_async(sys->smb2, sys->smb2fh, buf, len,
- smb2_read_cb, &op) < 0)
+ int err = smb2_read_async(sys->smb2, sys->smb2fh, buf, len,
+ smb2_read_cb, &op);
+ if (err < 0)
{
- VLC_SMB2_SET_ERROR(&op, "smb2_read_async", 1);
+ VLC_SMB2_SET_ERROR(&op, "smb2_read_async", err);
return 0;
}
- if (vlc_smb2_mainloop(&op, false) < 0)
- {
- sys->error_status = op.error_status;
+ if (vlc_smb2_mainloop(&op) < 0)
return 0;
- }
if (op.res.read.len == 0)
sys->eof = true;
@@ -340,16 +301,16 @@ FileSeek(stream_t *access, uint64_t i_pos)
{
struct access_sys *sys = access->p_sys;
- if (sys->error_status != 0)
+ if (sys->smb2 == NULL)
return VLC_EGENERIC;
- struct vlc_smb2_op op = VLC_SMB2_OP(access, sys->smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, &sys->smb2);
- if (smb2_lseek(op.smb2, sys->smb2fh, i_pos, SEEK_SET, NULL) < 0)
+ int err = smb2_lseek(op.smb2, sys->smb2fh, i_pos, SEEK_SET, NULL);
+ if (err < 0)
{
- VLC_SMB2_SET_ERROR(&op, "smb2_seek_async", 1);
- sys->error_status = op.error_status;
- return VLC_EGENERIC;
+ VLC_SMB2_SET_ERROR(&op, "smb2_seek_async", err);
+ return err;
}
sys->eof = false;
@@ -520,32 +481,34 @@ ShareEnum(stream_t *access, input_item_node_t *p_node)
}
static int
-vlc_smb2_close_fh(stream_t *access, struct smb2_context *smb2,
+vlc_smb2_close_fh(stream_t *access, struct smb2_context **smb2p,
struct smb2fh *smb2fh)
{
- struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2p);
- if (smb2_close_async(smb2, smb2fh, smb2_generic_cb, &op) < 0)
+ int err = smb2_close_async(op.smb2, smb2fh, smb2_generic_cb, &op);
+ if (err < 0)
{
- VLC_SMB2_SET_ERROR(&op, "smb2_close_async", 1);
+ VLC_SMB2_SET_ERROR(&op, "smb2_close_async", err);
return -1;
}
- return vlc_smb2_mainloop(&op, true);
+ return vlc_smb2_mainloop(&op);
}
static int
-vlc_smb2_disconnect_share(stream_t *access, struct smb2_context *smb2)
+vlc_smb2_disconnect_share(stream_t *access, struct smb2_context **smb2p)
{
- struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2p);
- if (smb2_disconnect_share_async(smb2, smb2_generic_cb, &op) < 0)
+ int err = smb2_disconnect_share_async(op.smb2, smb2_generic_cb, &op);
+ if (err < 0)
{
- VLC_SMB2_SET_ERROR(&op, "smb2_connect_share_async", 1);
+ VLC_SMB2_SET_ERROR(&op, "smb2_connect_share_async", err);
return -1;
}
- return vlc_smb2_mainloop(&op, true);
+ return vlc_smb2_mainloop(&op);
}
static void
@@ -590,55 +553,58 @@ vlc_smb2_print_addr(stream_t *access)
}
static int
-vlc_smb2_open_share(stream_t *access, struct smb2_context *smb2,
+vlc_smb2_open_share(stream_t *access, struct smb2_context **smb2p,
struct smb2_url *smb2_url, bool do_enum)
{
struct access_sys *sys = access->p_sys;
struct smb2_stat_64 smb2_stat;
- struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, smb2p);
int ret;
if (do_enum)
- ret = smb2_share_enum_async(smb2, smb2_open_cb, &op);
+ ret = smb2_share_enum_async(op.smb2, smb2_open_cb, &op);
else
{
- if (smb2_stat_async(smb2, smb2_url->path, &smb2_stat,
- smb2_generic_cb, &op) < 0)
- VLC_SMB2_SET_ERROR(&op, "smb2_stat_async", 1);
+ ret = smb2_stat_async(op.smb2, smb2_url->path, &smb2_stat,
+ smb2_generic_cb, &op);
+ if (ret < 0)
+ {
+ VLC_SMB2_SET_ERROR(&op, "smb2_stat_async", ret);
+ goto error;
+ }
- if (vlc_smb2_mainloop(&op, false) != 0)
+ if (vlc_smb2_mainloop(&op) != 0)
goto error;
if (smb2_stat.smb2_type == SMB2_TYPE_FILE)
{
- vlc_smb2_op_reset(&op, smb2);
+ vlc_smb2_op_reset(&op, smb2p);
sys->smb2_size = smb2_stat.smb2_size;
- ret = smb2_open_async(smb2, smb2_url->path, O_RDONLY,
+ ret = smb2_open_async(op.smb2, smb2_url->path, O_RDONLY,
smb2_open_cb, &op);
}
else if (smb2_stat.smb2_type == SMB2_TYPE_DIRECTORY)
{
- vlc_smb2_op_reset(&op, smb2);
+ vlc_smb2_op_reset(&op, smb2p);
- ret = smb2_opendir_async(smb2, smb2_url->path, smb2_open_cb, &op);
+ ret = smb2_opendir_async(op.smb2, smb2_url->path, smb2_open_cb, &op);
}
else
{
msg_Err(access, "smb2_stat_cb: file type not handled");
- op.error_status = 1;
- goto error;
+ ret = -ENOENT;
}
}
if (ret < 0)
{
- VLC_SMB2_SET_ERROR(&op, "smb2_open*_async", 1);
+ VLC_SMB2_SET_ERROR(&op, "smb2_open*_async", ret);
goto error;
}
- if (vlc_smb2_mainloop(&op, false) != 0)
+ if (vlc_smb2_mainloop(&op) != 0)
goto error;
if (do_enum)
@@ -652,8 +618,7 @@ vlc_smb2_open_share(stream_t *access, struct smb2_context *smb2,
return 0;
error:
- sys->error_status = op.error_status;
- return -1;
+ return op.error_status;
}
static void
@@ -661,8 +626,9 @@ vlc_smb2_FreeContext(void *context)
{
struct smb2_context *smb2 = context;
- vlc_smb2_disconnect_share(NULL, smb2);
- smb2_destroy_context(smb2);
+ vlc_smb2_disconnect_share(NULL, &smb2);
+ if (smb2 != NULL)
+ smb2_destroy_context(smb2);
}
static int
@@ -704,9 +670,12 @@ vlc_smb2_connect_open_share(stream_t *access, const char *url,
credential->psz_username);
if (cache_entry != NULL)
{
- int err = vlc_smb2_open_share(access, cache_entry->context, smb2_url, do_enum);
+ struct smb2_context *smb2 = cache_entry->context;
+ int err = vlc_smb2_open_share(access, &smb2, smb2_url, do_enum);
if (err == 0)
{
+ assert(smb2 != NULL);
+
smb2_destroy_context(sys->smb2);
sys->smb2 = cache_entry->context;
sys->smb2_connected = true;
@@ -716,40 +685,44 @@ vlc_smb2_connect_open_share(stream_t *access, const char *url,
msg_Dbg(access, "re-using old smb2 session");
return 0;
}
+ else
+ vlc_access_cache_entry_Delete(cache_entry);
}
smb2_set_security_mode(sys->smb2, SMB2_NEGOTIATE_SIGNING_ENABLED);
smb2_set_password(sys->smb2, password);
smb2_set_domain(sys->smb2, domain ? domain : "");
- struct vlc_smb2_op op = VLC_SMB2_OP(access, sys->smb2);
+ struct vlc_smb2_op op = VLC_SMB2_OP(access, &sys->smb2);
int err = smb2_connect_share_async(sys->smb2, smb2_url->server, share,
username, smb2_generic_cb, &op);
if (err < 0)
{
VLC_SMB2_SET_ERROR(&op, "smb2_connect_share_async", err);
- sys->error_status = op.error_status;
goto error;
}
- if (vlc_smb2_mainloop(&op, false) != 0)
- {
- sys->error_status = op.error_status;
+ if (vlc_smb2_mainloop(&op) != 0)
goto error;
- }
sys->smb2_connected = true;
vlc_smb2_print_addr(access);
- err = vlc_smb2_open_share(access, sys->smb2, smb2_url, do_enum);
+ err = vlc_smb2_open_share(access, &sys->smb2, smb2_url, do_enum);
if (err < 0)
+ {
+ op.error_status = err;
goto error;
+ }
sys->cache_entry = vlc_access_cache_entry_NewSmb(sys->smb2, smb2_url->server, share,
credential->psz_username,
vlc_smb2_FreeContext);
if (sys->cache_entry == NULL)
+ {
+ op.error_status = -ENOMEM;
goto error;
+ }
smb2_destroy_url(smb2_url);
return 0;
@@ -762,13 +735,16 @@ error:
{
if (sys->smb2_connected)
{
- vlc_smb2_disconnect_share(access, sys->smb2);
+ vlc_smb2_disconnect_share(access, &sys->smb2);
sys->smb2_connected = false;
}
- smb2_destroy_context(sys->smb2);
- sys->smb2 = NULL;
+ if (sys->smb2 != NULL)
+ {
+ smb2_destroy_context(sys->smb2);
+ sys->smb2 = NULL;
+ }
}
- return -1;
+ return op.error_status;
}
static char *
@@ -821,6 +797,7 @@ Open(vlc_object_t *p_obj)
stream_t *access = (stream_t *)p_obj;
struct access_sys *sys = vlc_obj_calloc(p_obj, 1, sizeof (*sys));
char *var_domain = NULL;
+ int ret;
if (unlikely(sys == NULL))
return VLC_ENOMEM;
@@ -853,10 +830,10 @@ Open(vlc_object_t *p_obj)
{
free(url);
free(resolved_host);
+ ret = -ENOMEM;
goto error;
}
- int ret = -1;
vlc_credential credential;
vlc_credential_init(&credential, &sys->encoded_url);
var_domain = var_InheritString(access, "smb-domain");
@@ -868,15 +845,11 @@ Open(vlc_object_t *p_obj)
NULL);
ret = vlc_smb2_connect_open_share(access, url, &credential);
- while (ret == -1
- && (!sys->error_status || VLC_SMB2_STATUS_DENIED(sys->error_status))
+ while (VLC_SMB2_STATUS_DENIED(ret)
&& vlc_credential_get(&credential, access, "smb-user", "smb-pwd",
SMB_LOGIN_DIALOG_TITLE, SMB_LOGIN_DIALOG_TEXT,
sys->encoded_url.psz_host))
- {
- sys->error_status = 0;
ret = vlc_smb2_connect_open_share(access, url, &credential);
- }
free(resolved_host);
free(url);
if (ret == 0)
@@ -932,8 +905,7 @@ error:
* case of network error (EIO) or when the user asked to cancel it
* (vlc_killed()). Indeed, in these cases, it is useless to try next smb
* modules. */
- return vlc_killed() || sys->error_status == -EIO ? VLC_ETIMEOUT
- : VLC_EGENERIC;
+ return vlc_killed() || ret == -EIO ? VLC_ETIMEOUT : VLC_EGENERIC;
}
static void
@@ -943,7 +915,10 @@ Close(vlc_object_t *p_obj)
struct access_sys *sys = access->p_sys;
if (sys->smb2fh != NULL)
- vlc_smb2_close_fh(access, sys->smb2, sys->smb2fh);
+ {
+ if (sys->smb2)
+ vlc_smb2_close_fh(access, &sys->smb2, sys->smb2fh);
+ }
else if (sys->smb2dir != NULL)
smb2_closedir(sys->smb2, sys->smb2dir);
else if (sys->share_enum != NULL)
@@ -953,7 +928,10 @@ Close(vlc_object_t *p_obj)
assert(sys->smb2_connected);
- vlc_access_cache_AddEntry(&smb2_cache, sys->cache_entry);
+ if (sys->smb2 != NULL)
+ vlc_access_cache_AddEntry(&smb2_cache, sys->cache_entry);
+ else
+ vlc_access_cache_entry_Delete(sys->cache_entry);
vlc_UrlClean(&sys->encoded_url);
}
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/ff0e681fedb81895e28bb97931c96f757110d099...ac95bf19f9991f506170a3dcb3c57268cb944727
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/ff0e681fedb81895e28bb97931c96f757110d099...ac95bf19f9991f506170a3dcb3c57268cb944727
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list